SLING-11458 - fixed a regresssion for the original Ticket SLING-9626

[schaefa]

The Json Writer that wraps the response writer must not be flushed or closed as this can lead to issues down the chain

[kwin]

I would suggest using https://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/output/CloseShieldWriter.html#wrap-java.io.Writer- instead. Then you can continue using try with resources because not calling close on JSONWriter is dangerous as it might perform other operations than just closing the underlying writer.

[schaefa]

@kwin First this does not work as it replaces the writer when closed is called but I also think that we should make it explicit there as this is important. Also SonarCloud is wrong here - I think.

[kwin]

First this does not work as it replaces the writer when closed is called

I am not following here. You can just use try (JsonWriter writer = Json.createWriter(CloseShieldWriter.wrap(response.getWriter()))) in

sling-org-apache-sling-graphql-core/src/main/java/org/apache/sling/graphql/core/servlet/GraphQLServlet.java

Line 318 in 9989848

try (JsonWriter writer = Json.createWriter(response.getWriter())) {

.

but I also think that we should make it explicit there as this is important.

I think using CloseShieldWriter is way more explicit than just not closing.

Also SonarCloud is wrong here - I think.

In this particular case maybe yes, but as stated above, it may be possible that in the future another implementation of JSONWriter may do necessary cleanup in its close() apart from just delegating to its underlying writer.

[schaefa]

@kwin The Close Shield Writer is not just preventing the close but it is replacing the writer:

public void close() {
    this.out = ClosedWriter.CLOSED_WRITER;
}

This will lead to failures in the IT tests of this module but I am pretty sure this will lead to issues when using it.

[kwin]

close() is the last method called on an instance of try with resources so I fail to see how this would have a negative impact. The CloseShieldWriter uses that to detect in case close is called twice, but it won’t have an effect on either the wrapped reader nor on any wrappers of CloseShieldWriter.

[schaefa]

@kwin You give it a try (on both execute() methods):
try (JsonWriter writer = Json.createWriter(CloseShieldWriter.wrap(response.getWriter()))) {

This will yield 4 IT test failure:

[ERROR] Errors:
[ERROR] GraphQLServletIT.testGqlExt:112->GraphQLCoreTestSupport.getContent:198->GraphQLCoreTestSupport.executeRequest:188 » IO
[ERROR] GraphQLServletIT.testMultipleSchemaProviders:245->GraphQLCoreTestSupport.getContent:198->GraphQLCoreTestSupport.executeRequest:188 » IO
[ERROR] GraphQLServletIT.testOtherExtAndOtherSelector:222->GraphQLCoreTestSupport.getContent:198->GraphQLCoreTestSupport.executeRequest:188 » IO
[ERROR] GraphQLServletIT.testOtherExtAndTestingSelector:216->GraphQLCoreTestSupport.getContent:198->GraphQLCoreTestSupport.executeRequest:188 » IO

No failure with my solution

[schaefa]

Updated the code to use a Wrapper to prevent the closure to satisfy the SonarCloud code check

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[raducotescu]

This issue should be fixed in Sling in a more generic way and not only in this module - see https://lists.apache.org/thread/lytsl0b23d56xkmvsvnoxm4d7skxcq5h for more details.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[sonarqubecloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud