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

Support for JAX-RS 2.2 Java SE Bootstrapping API #3839

Merged
merged 12 commits into from
Apr 12, 2019

Conversation

mkarg
Copy link
Member

@mkarg mkarg commented May 26, 2018

Implementation of the new Java SE Bootstrapping API of JAX-RS 2.2.

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch from d577dee to 89a66c7 Compare May 26, 2018 08:55
@mkarg
Copy link
Member Author

mkarg commented May 26, 2018

@jansupol This is only a very, very initial proof-of-concept I maintain in sync with my progress on the API drafts. If nobody objects or already is planned for, I would like to extend it in the next weeks to become a full-monty implementation of the Java SE Bootstrap API. Is that OK for your team or are other people working on this already?

@jansupol
Copy link
Contributor

@mkarg Noone started to work on this yet, since the JAXRS interface (I still do not enjoy the name - Does Bootstrap make more sense?) is not finished. We would welcome your PR, but first, Santiago has worked on some JAX-RS 2.2 features implementation and he will put this code to a separate branch, that expects JAX-RS 2.2. This PR should be placed in that branch as well.

@mkarg
Copy link
Member Author

mkarg commented May 30, 2018

@jansupol Great to hear, because I already have it halfway done (runs fine with Grizzly, Jetty and JDK already). So I will finish it, add tests and keep it in sync with the ugly-named interface.

@mkarg
Copy link
Member Author

mkarg commented Jun 1, 2018

@jansupol Regarding your recent comment on auto-selection of ports, I have two questions: (a) Do you mean auto-selection of port (= range scan), or do you mean reporting the native API's default port (= 80 on Jersey)? If you mean range scan, can you please point me to the code in Jersey that provides a port scanner?

@jansupol
Copy link
Contributor

jansupol commented Jun 1, 2018

The range scan would be great, would it not? But I would prefer to leave that functionality optional on the implementation builder and not make that mandatory that by JAX-RS. I just meant a single number, which the implementation can choose. The JAXRS interface implementation should set the port independently on the container implementations, to mandate that port on any (current or future) container the user chooses. I personally favour 8080.

@mkarg
Copy link
Member Author

mkarg commented Jun 1, 2018

Five years ago range scan would have been a great addition to Jersey, but with docker being umbiquitus and doing range scan on his own, I think only very few people will have an actual benefit of that at time of publishing Jersey 2.28. OTOH if someone likes to spent the time to add this feature, great, then I will +1 on that PR. :-)

If we really want to return a single number instead of getting one provided, then I wonder why Jersey shall select it globally instead of using any existing defaults already existing in the container providers? I mean, why shall we return 8080 in case someone explicitly requests Grizzly and that one has 80, or explicitly requests HTTPS on Jetty and that one picks 8333? That makes no sense to me. So either the default is 8080 defined by the JAX-RS specification, or it is whatever the container decided to use. Or I do not understand the purpose here?

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch from 25af659 to 5b47056 Compare June 2, 2018 18:25
@mkarg
Copy link
Member Author

mkarg commented Jun 2, 2018

@jansupol You convinced me. I Just updated the PoC. It now does auto-selection of 80 / 443 dependend of protocol HTTP / HTTPS. More code will follow next week.

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch 2 times, most recently from f809d30 to 92182ef Compare June 8, 2018 21:50
@mkarg mkarg changed the base branch from master to JAXRS_2_2 June 8, 2018 21:57
@mkarg
Copy link
Member Author

mkarg commented Jun 10, 2018

Supports sixth draft of JAXRS interface:

  • TLS configuration.

Tested using Grizzly2, Jetty, Netty, JDK-HTTP, Simple. For SimpleFramework mutual authentication (i. e. enabling SSL client authentication) is not yet implemented. I ran out of time and will look into that in the next days.

As requested, this PR now is based on the JAXRS_2_2 branch.

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch 3 times, most recently from 2e3ea37 to b76a6c3 Compare June 30, 2018 13:44
@jansupol
Copy link
Contributor

jansupol commented Jul 3, 2018

@mkarg This is rather large commit., sorry it took me so long to get to it. I had an idea to review all at once, but it seems too difficult. I try to add comments when I got hit by something.

@jansupol
Copy link
Contributor

jansupol commented Jul 3, 2018

Please do not forget the license headers.

@jansupol
Copy link
Contributor

jansupol commented Jul 3, 2018

The more I think about it the more I am convinced that Server should extend Container. The Server implementations can hold the Container that they create, so that the methods reload are accessible. Anyway, the Server hold the actual server implementation instances private (for instance grizzly HttpServer), I believe there should be a getter for these instances in the Server implementations.

@mkarg
Copy link
Member Author

mkarg commented Jul 4, 2018

I will have to check the actual code what it implies to implement Container. Thanks for pointing this out.

@mkarg
Copy link
Member Author

mkarg commented Jul 10, 2018

License headers and improved Javadocs will follow in a subsequent commit (I planned to do that at last, just wanted to get the actual code correct first).

I thought about Server implementing Container and just pushed a commit with a different design: It does not extend the container, but it provides it. I do not see that a server is a special form of a container from the birds view, but instead both are separate assets working together. So I hope you don't mind that I keep the individual concepts, but just added Server.container() so one can request the container via the server if needed.

Regarding you proposal to query the native server instance: This was already existing, see Server.unwrap().

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch 2 times, most recently from fdbfc3f to e896a0f Compare July 15, 2018 21:17
@mkarg
Copy link
Member Author

mkarg commented Jul 15, 2018

I will review my code this week. After that I will update Javadocs and licence headers, and squash outdated commits.

@jansupol
Copy link
Contributor

@mkarg I was thinking, the JAXRS.Configuration, should it be Configurable? It would allow to register contacts that the VIs can use in their native SE bootstrap implementations. What do you think?

@mkarg
Copy link
Member Author

mkarg commented Jul 16, 2018

@jansupol I would rather slow down with more additions to the JAXRS Bootstrap. For me implementing the 7th draft worked liked a charm for Jersey and a PoC was hacked within 30 minutes in RESTeasy. So maybe it would be a good idea to first merge the 7th draft (+ docs + tests) and collect ideas for a future PR after the vendors are done with implementing the 7th draft? It makes decision easier having something in the hands to discuss about. Such a "post-implementation-PR" could also cover ideas like thread pools and optional support for CDI 2.0 SeContainer (RESTeasy is working an that AFAIK). Besides that, my gut instinct say that Configurable is something different to JAXRS.Configuration (one is the application, the other is the boilerplate; I'd rather not mix it).

@jansupol
Copy link
Contributor

@mkarg Please go ahead. If we should ever finish this review, it would be good to have it buildable.

Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
@mkarg
Copy link
Member Author

mkarg commented Feb 23, 2019

@jansupol Rebased Java SE Bootstrap API ontop of branch JAXRS_2_2. I kindly request to continue the review. :-)

@mkarg
Copy link
Member Author

mkarg commented Mar 8, 2019

@jansupol What is holding you back from merging this? Anything you want me to change?

@mkarg mkarg force-pushed the wip-poc-javase-bootstrap branch 2 times, most recently from bd09302 to d74e42b Compare March 28, 2019 18:53
This reverts commit 0441f3a.

Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
Signed-off-by: Markus KARG <markus@headcrashing.eu>
@mkarg
Copy link
Member Author

mkarg commented Mar 28, 2019

Do not create integration with microprofile. We can create the integration in some extension module, but not as a default in jaxrs-ri. The provided scope does not solve this. Let's focus on the bootstrap iteslf for now, we can deal with the mp config later. In a separate module.

All requested changes are made. I would be more than happy to see this review being finished and the changes finally merged into the `JAXRS_2_2' branch. Otherwise please tell me what more you want me to change. :-)

@mkarg
Copy link
Member Author

mkarg commented Mar 29, 2019

@arjantijms @asoldano @andymc12 Review please. :-)

@jansupol jansupol added the 2.30 label Mar 29, 2019
@jansupol
Copy link
Contributor

@mkarg It looks good. Thank you.

@jansupol jansupol merged commit d965220 into eclipse-ee4j:JAXRS_2_2 Apr 12, 2019
@mkarg mkarg deleted the wip-poc-javase-bootstrap branch April 12, 2019 13:49
@mkarg mkarg restored the wip-poc-javase-bootstrap branch April 12, 2019 21:10
@jansupol jansupol removed the 2.30 label Jul 28, 2021
@jansupol jansupol added this to the 3.1.0 milestone Jul 28, 2021
@jansupol jansupol removed this from the 3.1.0 milestone Sep 7, 2021
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

Successfully merging this pull request may close these issues.

3 participants