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

SLING-12027 - GraphQL core fails to build with Java 17 #38

Merged
merged 5 commits into from Sep 27, 2023

Conversation

rombert
Copy link
Contributor

@rombert rombert commented Sep 19, 2023

No description provided.

Update to the latest version of o.a.f.framework
Update to the latest version of o.a.s.testing.paxexam
@rombert
Copy link
Contributor Author

rombert commented Sep 19, 2023

@raducotescu @schaefa - I think this is mostly done, but one test fails and it requires more GraphQL insight

[ERROR] Failures:
[ERROR] GraphQLServletIT.testPersistingInvalidQueries:159 Did not expect to persist an invalid query. expected:<400> but was:<500>

It would be great if you could look into this so we can have Java 17 compatibility for this bundle.

@schaefa
Copy link
Contributor

schaefa commented Sep 19, 2023

@rombert I will check it out

@schaefa
Copy link
Contributor

schaefa commented Sep 20, 2023

There is a problem where the initial Error Code of 400 is turned into an Internal Server Error (500). This is happening between setting the error code SlingServletResolver.handleError() line 387 and obtaining the Error Code in DefaultErrorHandlerServlet.service() line 70. Because it cannot find it it will set the error code to 500.

@raducotescu
Copy link
Member

raducotescu commented Sep 26, 2023

@schaefa, the code fails on Java 11 in the same way, which means that there is a bug or a behaviour change in one of the updated libraries.

@@ -249,7 +249,7 @@
<dependency>
<groupId>org.apache.sling</groupId>
<artifactId>org.apache.sling.testing.paxexam</artifactId>
<version>3.1.0</version>
<version>4.0.0</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the culprit of the failing test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update brought org.apache.felix.jetty-5.0.0, which uses Jetty 11. Unfortunately, there's a Servlet Request Wrapper in Felix that doesn't correctly return the set attributes if they were RequestDispatcher attributes - see https://github.com/apache/felix-dev/blob/d691a59c9e78749fad20a10a002c969e4e583800/http/wrappers/src/main/java/org/apache/felix/http/javaxwrappers/ServletRequestWrapper.java#L108-L182

@cziegeler will fix this in Felix and for this test setup we could override the Jetty version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

* use o.a.felix.http.jetty 4.x until FELIX-6656 is fixed
* cleaned some redundant dependencies, now that we updated to
o.a.sling.testing.paxexam 4.0.0
@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rombert rombert merged commit e370de6 into master Sep 27, 2023
3 checks passed
@rombert rombert deleted the issue/SLING-12027 branch September 27, 2023 08:36
@rombert
Copy link
Contributor Author

rombert commented Sep 27, 2023

Thanks @schaefa , @raducotescu , @cziegeler for your help, this has been a fun ride :-)

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