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

PAYARA-4164 Robust null-safe code #4241

Merged

Conversation

@dmatej
Copy link
Contributor

dmatej commented Sep 26, 2019

  • invocationManager is null for default admin context
  • there is no guarantee to have all needed services accessible
  • application name can be null for context of server modules

Testing

Testing Performed

Fixes TCK/CDI

Test suites executed

  • [x ] Quicklook
  • Payara Samples
  • Java EE7 Samples
  • Java EE8 Samples
  • Payara Private Tests
  • Payara Microprofile TCKs Runner
  • Jakarta TCKs
  • Mojarra
  • Cargo Tracker
- invocationManager is null for default admin context
- there is no guarantee to have all needed services accessible
- application name can be null for context of server modules
@dmatej dmatej requested review from pdudits and Pandrex247 Sep 26, 2019
@dmatej dmatej self-assigned this Sep 26, 2019
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor

svendiedrichsen commented Sep 26, 2019

@dmatej You could use java.util.Optional to clean up code from all the NULL checks.

@dmatej

This comment has been minimized.

Copy link
Contributor Author

dmatej commented Sep 26, 2019

@svendiedrichsen Then I would need to combine two optionals, Java 9 has Optional.or not sure how to do that in Java 8, but I'll try to google it tomorrow :)
How would you do that?

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor

svendiedrichsen commented Sep 26, 2019

@dmatej I have tried on the OpenTracingService.getApplicationName method. It doesn't look too pretty. You probably shouldn't do it.

    public String getApplicationName(final InvocationManager invocationManager) {
        final ComponentInvocation currentInvocation = invocationManager.getCurrentInvocation();
        return Optional.ofNullable(currentInvocation.getAppName())
                .orElseGet(() ->
                    Optional.ofNullable(currentInvocation.getModuleName())
                        .orElseGet(() ->
                            Optional.ofNullable(currentInvocation.getComponentId())
                                .filter(name ->
                                    Globals.getDefaultBaseServiceLocator().getService(ApplicationRegistry.class).get(name) == null
                                )
                                .map(name -> name.split("_/")[0])
                                .orElse(null)
                        )
                );
    }
@sgflt

This comment has been minimized.

Copy link

sgflt commented Sep 27, 2019

I would use this:

final String appName = Stream.of(
  invocation.getAppName(),
  invocation.getModuleName()
).filter(Objects::nonNull)
.findFirst()
.orElseGet(() -> buildAppNameFromComponentId(invocation.getComponentId()))

@dmatej

This comment has been minimized.

Copy link
Contributor Author

dmatej commented Sep 27, 2019

I thought we are talking about OpenTracingApplicationEventListener, not OpenTracingService. Then I would rewrite the whole file in the same style ;)

if (invocationManager == null || this.openTracing == null) {
this.applicationName = null;
} else {
this.applicationName = this.openTracing.getApplicationName(invocationManager);

This comment has been minimized.

Copy link
@jbee

jbee Sep 27, 2019

Contributor

I don't know if this is a personal preference but I'd use a conditional if I had two cases to initialise the same variable.

this.applicationName = <test>
  ? <a>
  : <b>

Makes it more clear that there is a section of code and after the variable is initialised to a value.

This comment has been minimized.

Copy link
@dmatej

dmatej Oct 4, 2019

Author Contributor

Hmm. It is very similar, but I looked at it after another week and the if/else was optically a bit more straightforward. I use conditionals most time only if it is single true/false condition. More complicated logic => method/function. Two lines ok, three lines warning sign :-)

 this.applicationName = invocationManager == null || this.openTracing == null ? null
            : this.openTracing.getApplicationName(invocationManager);
@jbee

This comment has been minimized.

Copy link
Contributor

jbee commented Sep 27, 2019

Please be careful with Optional. In the particular situation I don't think it improves the situation at all. It makes the code just appear much more complex than it actually is. I'd rather stick the candidates in a list and flatten the list to the first non null value or something like that. Or just do the cascading null check. Not the nicest code but it is very clear what it does. Takes a second to understand if you see it first time. Can't say the optional construct has the same quality.

@dmatej

This comment has been minimized.

Copy link
Contributor Author

dmatej commented Oct 1, 2019

Jenkins test please

@pdudits
pdudits approved these changes Oct 4, 2019
Copy link
Contributor

pdudits left a comment

Good for the time being, however I am very curious why existing check for actual EE application being deployed didn't kick in.
We need not to find out now, but there's surely an interesting race condition hiding.

@dmatej dmatej merged commit 8193389 into payara:master Oct 4, 2019
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
@dmatej dmatej deleted the dmatej:PAYARA-4164-fixed-npe-opentracing-for-master branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.