[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL #1649

liweinan · 2018-08-06T03:53:07Z

No description provided.

liweinan · 2018-08-06T03:53:26Z

work in progress

liweinan · 2018-08-06T03:55:59Z

Design:

liweinan · 2018-08-06T12:37:58Z

External grammar can be included now:

The left tasks is allow server to serve the external schema.

liweinan · 2018-08-06T17:17:29Z

ResteasyWadlGrammar is the class for grammar storing(both included grammars or generated grammars):

And it's designed to be thread-safe.

liweinan · 2018-08-06T17:19:26Z

Sample included schema output:

The usage should be simple:

contextBuilder.getDeployment().getActualResourceClasses().add(ResteasyWadlExtendedResource.class);

ResteasyWadlGrammar.includeGrammars("application-grammars.xml");

As shown above, the included grammars should be put into classpath and set via includeGrammars method.

liweinan · 2018-08-07T09:17:02Z

Now the schema generation is working:

It will scan the resource methods and its parameters(also return types), and collect all @XmlRootElement annotated classes, and generate schemas for them.

To use the generation function, just enable it:

ResteasyWadlGrammar.enableSchemaGeneration();

And it will be automatically included in grammars section:

liweinan · 2018-08-07T09:20:12Z

The next step is to cleanup the code and add the tests. Then I'll move this PR for review.

liweinan · 2018-08-08T07:34:22Z

Final design:

tests added. code cleanup done.

rsvoboda · 2018-08-13T16:02:07Z

Just scanned the PR a bit, no deep dive.

please remove System.out.println
hard-coded http://127.0.0.1 can bring issues in the future
- HttpServer is used instead of ARQ in resteasy-wadl module

liweinan · 2018-08-14T04:18:14Z

@rsvoboda thanks for checking!

liweinan · 2018-08-14T12:31:36Z

@rsvoboda @asoldano To move tests to integration-tests and using ARQ instead of the current standalone tests, we have to add resteasy-wadl module into jboss-modules:

wdyt?

asoldano

@liweinan , thanks for the PR. Now, besides for the minor comment below, I think that my main concern is with the lifecycle and and visibility of the new ResteasyWadlGrammar class. Is there a reason for having such a singleton? Can't we simply keep track of the relevant data in each deployment/classloader?

asoldano · 2018-09-04T09:48:33Z

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlGrammar.java

+    private static AtomicBoolean generateSchema = new AtomicBoolean(false);
+
+
+    public static boolean hasGrammars() {


Shouldn't this hasGrammar() be syncronized too (under the current design)?

asoldano · 2018-09-04T09:58:49Z

@liweinan , as for the resteasy-wadl new module, yes, that will have to be added

liweinan · 2018-09-05T07:16:36Z

thanks for reviewing everyone! Let me do the refactor work.

liweinan · 2018-10-19T06:06:53Z

Refactor

ResteasyWadlGrammar is no longer a singleton class and it's now inner class of ResteasyWadlWriter
ResteasyWadlResourceHelper is added to hold the singletion instance of ResteasyWadlWriter(the instance is moved out of ResteasyWadlDefaultResource).
ResteasyWadlDefaultResource and ResteasyWadlExtendedResource are provided for default WADL usages, and they use the writer inside ResteasyWadlResourceHelper.
ResteasyWadlWriter supports caching now(And it's turned off by default).

Besides, hard coded port numbers are cleaned up in tests.

Usage Becomes

Add ResteasyWadlDefaultResource and ResteasyWadlExtendedResource into deployment:

contextBuilder.getDeployment().getActualResourceClasses().add(ResteasyWadlDefaultResource.class);

contextBuilder.getDeployment().getActualResourceClasses().add(ResteasyWadlExtendedResource.class);

Actually the usages ofResteasyWadlDefaultResource, ResteasyWadlExtendedResource and ResteasyWadlResourceHelper are all optional. But it's convenient to use them because it helps users to setup almost everything. And it will provide two paths: /application.xml and /wadl-extended/{path} to the users.

To use grammar support, it's necessary to create an instance of ResteasyWadlGrammar. The recommended way is to put it into ResteasyWadlResourceHelper.apiWriter:

ResteasyWadlWriter.ResteasyWadlGrammar wadlGrammar = new ResteasyWadlWriter.ResteasyWadlGrammar();

// if you want to use your own handwritten grammar file:
wadlGrammar.includeGrammars("application-grammars.xml");

// if you'd like to enable automatic schema generation:
wadlGrammar.enableSchemaGeneration();

Put it into ResteasyWadlResourceHelper for future usage:

ResteasyWadlResourceHelper.getApiWriter().setWadlGrammar(wadlGrammar);

Then ResteasyWadlDefaultResource and ResteasyWadlExtendedResource can use it properly.

liweinan · 2018-10-19T06:47:11Z

Relative PRs:

RESTEASY-1360 - Create RESTEasy WADL Feature pack for WildFly
RESTEASY-2043 - Update jboss-modules to include restasy-wadl module

@asoldano I have created separated JIRA tasks to work on jboss-module / wildfly integration of WADL. So this PR is complete now. Could you please help to review it? Thanks!

asoldano · 2018-10-22T13:25:48Z

@liweinan please start by rebasing to resolve the conflicts and then removing the "requires rebase" label ;-)

asoldano · 2018-10-22T16:04:40Z

@liweinan nevermind, the PR now shows no conflicts

asoldano

@liweinan I've checked the changes again. Honestly, I don't understand what has really changed wrt my previous comment. We have a single instance of ResteasyWadlWriter which relies on the latest and globally set grammar object. Is that really ok? what if 2 deployments runs in parallel? May be I really need to see an integration test using Arquillian to understand this. Can you clarify a bit?
Besides this, there're a couple of other things to solve, see comments below. Thanks!

asoldano · 2018-10-22T16:13:07Z

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlWriter.java

-import org.jboss.resteasy.wadl.jaxb.Resource;
-import org.jboss.resteasy.wadl.jaxb.Resources;
-import org.jboss.resteasy.wadl.jaxb.Response;
+import org.jboss.resteasy.wadl.jaxb.*;


Please do not use * imports

asoldano · 2018-10-22T16:14:43Z

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlWriter.java

+           } catch (JAXBException e) {
+               e.printStackTrace();
+           } catch (IOException e) {
+               e.printStackTrace();


Can we do something better for exception handling here? (at least use proper logging)

liweinan · 2018-10-23T07:02:18Z

@asoldano Thanks for checking! Let me:

Review the design and make necessary refactor
Provide the integration tests with Wildfly
Fix the imports

rsvoboda · 2018-10-23T10:27:23Z

@liweinan I think the main item is to provide details and explanation for this:

We have a single instance of ResteasyWadlWriter which relies on the latest and globally set grammar object. Is that really ok? what if 2 deployments runs in parallel? ... Can you clarify a bit?

liweinan · 2018-10-23T10:32:20Z

@rsvoboda Yeah that's a problem. Let me redesign this part.

liweinan · 2018-10-26T10:01:03Z

Changes

Fix the original global instance design of wadl writers, and now its instance could be put into per-deployment scope.
Add wildfly integration test.
Deprecate ResteasyWadlServlet and ResteasyWadlServletWriter, because it doesn't support grammars.
Improve ResteasyWadlDefaultResource, for it can be used in both embedded container and servlet container.
Using logging error instead of runtime exceptions inside wadl writer.
Other minor fixes.

liweinan · 2018-10-26T16:54:43Z

@asoldano The refactor is done. Could you please help to review it? Thanks!

refactor done

asoldano

Thanks @liweinan , this looks better. I have a question and a minor change requirement, please see the other comments. Thanks.

testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/wadl/WadlTestApplication.java

asoldano · 2018-10-27T10:13:42Z

...asy-wadl-undertow-connector/src/main/java/org/jboss/resteasy/wadl/WadlUndertowConnector.java

+//            .setAsyncSupported(false)
+//            .setLoadOnStartup(1)
+//            .addMapping("/application.xml");
+//      di.addServlet(resteasyWadlServlet);


Can we please get rid of this and other commented out stuff in this PR please?

Hi Alessio, I have added comments and removed the outdated code. Could you please help to review? Thanks!

testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/wadl/WadlTestApplication.java

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlServlet.java

asoldano · 2018-10-29T13:51:35Z

resteasy-wadl/src/test/java/org/jboss/resteasy/test/nextgen/wadl/WadlTests.java

      contextBuilder.bind(httpServer);
-      ResteasyWadlDefaultResource.getServices().put("/", ResteasyWadlGenerator.generateServiceRegistry(contextBuilder.getDeployment()));
+//      defaultResource.getServices().put("/", ResteasyWadlGenerator.generateServiceRegistry(contextBuilder.getDeployment()));


Can we also remove the commented out lines here above or do they have some special meaning that justify their presence?

testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/wadl/DeploymentTest.java

Hi Alessio, I have added comments about TestApplication. Could you please help to check if it's okay? Thanks!

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlWriter.java

liweinan self-assigned this Aug 6, 2018

liweinan added main Do Not Merge labels Aug 6, 2018

liweinan force-pushed the RESTEASY-1695 branch from cff664a to 3adfb8b Compare August 6, 2018 13:20

liweinan force-pushed the RESTEASY-1695 branch from a11d2db to efec8e2 Compare August 7, 2018 05:31

liweinan force-pushed the RESTEASY-1695 branch 2 times, most recently from 804654d to 3f193ad Compare August 8, 2018 07:32

liweinan removed the Do Not Merge label Aug 8, 2018

liweinan requested a review from asoldano August 8, 2018 07:44

asoldano reviewed Sep 4, 2018

View reviewed changes

liweinan force-pushed the RESTEASY-1695 branch from 3f193ad to 71dced8 Compare September 21, 2018 10:21

asoldano added the Requires rebase label Oct 12, 2018

liweinan force-pushed the RESTEASY-1695 branch from 71dced8 to 05f22c7 Compare October 17, 2018 10:20

liweinan force-pushed the RESTEASY-1695 branch 2 times, most recently from 96c7f67 to 35a0813 Compare October 19, 2018 06:40

asoldano removed the Requires rebase label Oct 22, 2018

asoldano previously requested changes Oct 22, 2018

View reviewed changes

liweinan force-pushed the RESTEASY-1695 branch from 0289bf8 to 9c90339 Compare October 26, 2018 09:52

liweinan force-pushed the RESTEASY-1695 branch from 976358f to 0866173 Compare October 27, 2018 00:33

asoldano previously requested changes Oct 27, 2018

View reviewed changes

liweinan force-pushed the RESTEASY-1695 branch from 0866173 to 7e8ae6f Compare October 29, 2018 02:17

[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL

3e63056

liweinan force-pushed the RESTEASY-1695 branch from 7e8ae6f to 3e63056 Compare October 29, 2018 03:52

asoldano previously requested changes Oct 29, 2018

View reviewed changes

liweinan added 2 commits October 30, 2018 10:34

.

c4be737

.

f354888

asoldano approved these changes Oct 30, 2018

View reviewed changes

asoldano merged commit 9207919 into resteasy:master Oct 30, 2018

tterem reviewed Oct 31, 2018

View reviewed changes

resteasy-wadl/src/main/java/org/jboss/resteasy/wadl/ResteasyWadlWriter.java Show resolved Hide resolved

tterem mentioned this pull request Oct 31, 2018

[RESTEASY-1974] Checkstyle - final fixes #1763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL #1649

[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL #1649

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 7, 2018 •

edited

liweinan commented Aug 7, 2018

liweinan commented Aug 8, 2018 •

edited

rsvoboda commented Aug 13, 2018

liweinan commented Aug 14, 2018

liweinan commented Aug 14, 2018

asoldano left a comment

asoldano Sep 4, 2018 •

edited

asoldano commented Sep 4, 2018

liweinan commented Sep 5, 2018

liweinan commented Oct 19, 2018 •

edited

liweinan commented Oct 19, 2018 •

edited

asoldano commented Oct 22, 2018

asoldano commented Oct 22, 2018

asoldano left a comment

asoldano Oct 22, 2018

asoldano Oct 22, 2018

liweinan commented Oct 23, 2018

rsvoboda commented Oct 23, 2018

liweinan commented Oct 23, 2018

liweinan commented Oct 26, 2018

liweinan commented Oct 26, 2018

asoldano left a comment

asoldano Oct 27, 2018

liweinan Oct 29, 2018

asoldano Oct 29, 2018

		private static AtomicBoolean generateSchema = new AtomicBoolean(false);


		public static boolean hasGrammars() {

[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL #1649

[RESTEASY-1695] Add GRAMMARS into RESTEasy WADL #1649

Conversation

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 6, 2018

liweinan commented Aug 7, 2018 • edited

liweinan commented Aug 7, 2018

liweinan commented Aug 8, 2018 • edited

rsvoboda commented Aug 13, 2018

liweinan commented Aug 14, 2018

liweinan commented Aug 14, 2018

asoldano left a comment

Choose a reason for hiding this comment

asoldano Sep 4, 2018 • edited

Choose a reason for hiding this comment

asoldano commented Sep 4, 2018

liweinan commented Sep 5, 2018

liweinan commented Oct 19, 2018 • edited

Refactor

Usage Becomes

liweinan commented Oct 19, 2018 • edited

asoldano commented Oct 22, 2018

asoldano commented Oct 22, 2018

asoldano left a comment

Choose a reason for hiding this comment

asoldano Oct 22, 2018

Choose a reason for hiding this comment

asoldano Oct 22, 2018

Choose a reason for hiding this comment

liweinan commented Oct 23, 2018

rsvoboda commented Oct 23, 2018

liweinan commented Oct 23, 2018

liweinan commented Oct 26, 2018

Changes

liweinan commented Oct 26, 2018

asoldano left a comment

Choose a reason for hiding this comment

asoldano Oct 27, 2018

Choose a reason for hiding this comment

liweinan Oct 29, 2018

Choose a reason for hiding this comment

asoldano Oct 29, 2018

Choose a reason for hiding this comment

liweinan commented Aug 7, 2018 •

edited

liweinan commented Aug 8, 2018 •

edited

asoldano Sep 4, 2018 •

edited

liweinan commented Oct 19, 2018 •

edited

liweinan commented Oct 19, 2018 •

edited