SWARM-1471: Do not auto-generate the JAX-RS application if the application-path has not been defined in YAML. #847
Conversation
| addGeneratedApplication(archive.as(JAXRSArchive.class)); | ||
| if (applicationPath.isExplicit()) { | ||
| addGeneratedApplication(archive.as(JAXRSArchive.class)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the only change that was required. All the other changes pertain to the test cases.
|
Haven’t reviewed but I think this is a good change. We just need to manage
expectations since it would be breaking.
…On Wed, Mar 21, 2018 at 9:08 PM Harsha Ramesh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
fractions/javaee/jaxrs/src/main/java/org/wildfly/swarm/jaxrs/runtime/DefaultApplicationDeploymentProcessor.java
<#847 (comment)>
:
> @@ -76,7 +76,9 @@ public void process() throws Exception {
return;
}
- addGeneratedApplication(archive.as(JAXRSArchive.class));
+ if (applicationPath.isExplicit()) {
+ addGeneratedApplication(archive.as(JAXRSArchive.class));
+ }
}
This is the only change that was required. All the other changes pertain
to the test cases.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#847 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA-TzdatNQt-b0stPG7lY4t95EFF7x_ks5tgvmCgaJpZM4S2SzH>
.
|
|
In addition to the release notes, the JAX-RS framework would automatically print a message that there are no Application subclasses or Servlet mappings present. It might be a manageable scenario. |
| @@ -76,7 +76,9 @@ public void process() throws Exception { | |||
| return; | |||
| } | |||
|
|
|||
| addGeneratedApplication(archive.as(JAXRSArchive.class)); | |||
| if (applicationPath.isExplicit()) { | |||
There was a problem hiding this comment.
Should probably change a bit the @AttributeDocumentation of applicationPath, to reflect the change.
There was a problem hiding this comment.
Let me take a stab at providing some useful documentation for the annotation. Will need a customer focussed doc-review of that line though.
There was a problem hiding this comment.
It's a single sentence, not an overly big deal. My suggestion: If set, WildFly Swarm will automatically generate a JAX-RS Application class and use this value as the @ApplicationPath
There was a problem hiding this comment.
Since I haven't looked at the existing documentation that much, I wasn't aware of the tone / technical details that are included in the documentation. Will use the above sentence and update.
|
I myself would create the |
|
I agree it's fine to use the yml version for tests, but the howto's should create the |
| @@ -0,0 +1,5 @@ | |||
| swarm: | |||
There was a problem hiding this comment.
Should this contain the port setting from the removed project-defaults.yml?
There was a problem hiding this comment.
Not really. The file was (for some reason) not being picked up. The test case was still pointing to 8080.
| @@ -0,0 +1,5 @@ | |||
| swarm: | |||
There was a problem hiding this comment.
Not really. The file was (for some reason) not being picked up. The test case was still pointing to 8080.
| @@ -76,7 +76,9 @@ public void process() throws Exception { | |||
| return; | |||
| } | |||
|
|
|||
| addGeneratedApplication(archive.as(JAXRSArchive.class)); | |||
| if (applicationPath.isExplicit()) { | |||
There was a problem hiding this comment.
Let me take a stab at providing some useful documentation for the annotation. Will need a customer focussed doc-review of that line though.
…ation-path has not been defined in YAML. Motivation ---------- Currently WF Swarm auto-generates a JAX-RS application class which leads to an inconsistent behavior (does not comply with the JAX-RS spec). Developers tend to get confused with the presence of multiple Application references being printed in the console. The intention of this auto-generation was to help developers easily define the JAX-RS application-path declaratively without needing to define a superfluous empty class. As discussed in the Jira issue, we should be generating the class only if the declarative parameter is defined. Modifications ------------- 1. Add the Application subclass only if no sub-class of Application is present **and** the YAML file defines the "application-path" entry. 2. Updated the deployment definitions for various test cases that were relying on this implicit behavior. 3. Updated the howto projects to include an Application subclass. Result ------ Verified the behavior (across many, many test cases).
|
Incorporate the changes. Executed a build from the |
|
A little less than 3 hours for completion of the builds. I wonder if we can optimize that! Anyways, ready for merging. |
|
@kenfinnigan @bobmcwhirter Is this PR good to be merged? Do you have any additional comments about it? Or should we put it on hold? |
Motivation
Currently WF Swarm auto-generates a JAX-RS application class which leads to an inconsistent behavior (does not comply with the JAX-RS spec). Developers tend to get confused with the presence of multiple Application references being printed in the console. The intention of this auto-generation was to help developers easily define the JAX-RS application-path declaratively without needing to define a superfluous empty class. As discussed in the Jira issue, we should be generating the class only if the declarative parameter is defined.
Modifications
Result
Verified the behavior (across many, many test cases).
mvn clean install?