Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Glassfish dependencies from the API. #22

Open
garretwilson opened this issue Jan 2, 2015 · 6 comments
Open

Remove Glassfish dependencies from the API. #22

garretwilson opened this issue Jan 2, 2015 · 6 comments

Comments

@garretwilson
Copy link

This is a very nice start to a framework. But although it is natural that restito would use some server dependency under the covers, the specifics of this server should not be promoted to the API. That is, I should be able to use restito without knowing what server implementation is used.

For instance, restito's Condition.method(Method) method requires a Method parameter, but the org.glassfish.grizzly.http.Method type is part of Glassfish. It would be better to use a string parameter, as in Condition.method(String). This way my code can be completely server agnostic. In addition, I can use more standard method constants, such as javax.ws.rs.Httpmethod.

@mkotsur
Copy link
Owner

mkotsur commented Jan 2, 2015

Hi,

Thanks for the suggestion. I've been thinking about breaking Restito into modules recently. That was more about: removing junit support from the 'core' module on one hand, and adding more junit-related features into 'restito-junit' module.

Will try to do the same for grizzly.

@garretwilson
Copy link
Author

Hi, @mkotsur . Since I reported this issue, I'm introducing Restito to another large client of mine as well as writing a software developer course which will feature Restito in the JAX-RS client lesson. I was just now writing the lesson and came upon Condition.method(org.glassfish.grizzly.http.Method m). I was getting ready to file an issue, but then I discovered that I already did, over two and a half years ago!

It's disappointing that a Glassfish dependency is still mixed into this great library. Do you have any idea of whether you can get to this any time soon?

If not, if I devote a developer to this project to remove this dependency, would you be open to accepting our pull request and removing that dependency altogether?

Thanks for your consideration.

@garretwilson
Copy link
Author

I think what would be more appropriate would be Condition.method(String name, String uri). Then if you want to create a Glassfish Method (however that works) behind the scenes, fine. But we don't want to introduce Glassfish-related code into our test code.

See for example the pattern used by the JAX-RS client code:

SyncInvoker.method(String name)

@garretwilson
Copy link
Author

(P.S. You really need methods taking URI as well to indicate the URIs.)

@mkotsur
Copy link
Owner

mkotsur commented Sep 28, 2017

Hello @garretwilson . Cool that you keep using restito despite lack of progress with this issue :-) It hasn't been ignored, though. I've made a research, and decided that this non-trivial breaking change doesn't justify the benefit. But I'm open for revisiting this decision.

So, just to make sure we're on the same page: Restito uses Glassfish for 2 purposes:

  • To run a web-server (which is essential for what Restito does). It would be possible to split restito into restito-core and restito-glassfish, and make restito-core free of glassfish. But to make the framework work you would need to have both! Unless you implement some interfaces from restito-core with the web-server of your choice. Is this what you'd like to be able to do?

  • To have some type-safe method parameters. Like in public static Condition method(final Method m), or public static Action status(final HttpStatus status). May be for the method it doesn't matter that much, people mostly know them by heart, but, I guess, you would agree that status(HttpStatus.NO_CONTENT_204) reads better than status(204). Without a doubt, for each such a method we could easily create an alternative, which takes a plain value (string or int in these cases). Then you would never need to import anything from org.glassfish.grizzly in your test code. The dependency would still be there, however, and it would still run Glassfish under the hood.

@garretwilson
Copy link
Author

The dependency would still be there, however, and it would still run Glassfish under the hood.

First of all, let me just say that I completely understand this part; I'm not trying to do away with Glassfish "under the hood".

But remember that developers will probably already be using some framework to do client REST calls. Now they will need to learn Restito, which is fine. I just don't want them to be forced to use yet another framework that they don't even need to know about.

you would agree that status(HttpStatus.NO_CONTENT_204) reads better than status(204)

Yes of course I agree. But you are assuming Glassfish is the only library that provides those constants. There is no guarantee that users of Restito are already using Glassfish. In fact they are probably not.

What is much more likely is that users are using JAX-RS for the client code. JAX-RS provides (as part of Java EE) a javax.ws.rs.core.Response.Status enum. So for many (perhaps most) Java developers, the question isn't between using status(204) or status(HttpStatus.NO_CONTENT_204); it is between using status(Response.Status.NO_CONTENT.getStatusCode()) (which they are probably already using, both on the server and on the client), or being forced to use status(HttpStatus.NO_CONTENT_204), which uses a class HttpStatus which they've never heard of, which uses a server Glassfish which they've never used, and which relies on some dependencies that they didn't even know were being pulled in and which they will have to go research to even find out how to access.

In fact, these constants are supplied in various libraries already; see e.g. https://stackoverflow.com/q/730283/421049 .

So if you just have to include methods with Glassfish classes, please at least supply the bare string/int versions as well so that I, my developers, and my students don't have to go digging through yet another library and we can use the library we're already using for our constants. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants