search

cloudfoundry / cf-java-client

Java Client Library for Cloud Foundry
Apache License 2.0
328 stars 318 forks source link

Avoid NPE in DefaultApplications.java #1126

Closed marc-schaefers closed 2 years ago

marc-schaefers commented 3 years ago

avoid NPE, if environmentJsons is null from application entity

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

dmikusa commented 3 years ago

@marc-schaefers - Thanks for the submission! Are you seeing this NPE in actual usage of the library? Do you have a stack trace you can share? Just looking for a little more context on this.

Also, you need to click through the links above and appease the EasyCLA bot before we can accept this PR.

marc-schaefers commented 3 years ago

Hi, i use the PushApplicationRequest to push a static build pack with the cloud foundry java libs. I want to push it on a customer cloud foundry environment, so i haven't configure it or anything.

On one app name i get a nullpointer exception: ERROR 18.11.2021 09:22:23.296 (de.aboutcontent.cloudfoundry.deploy.DeployExecutable): error on push java.lang.NullPointerException at org.cloudfoundry.operations.applications.DefaultApplications.lambda$getApplicationId$67(DefaultApplications.java:793) Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: Assembly trace from producer [reactor.core.publisher.MonoIgnoreElements] : reactor.core.publisher.Mono.checkpoint(Mono.java:1902) org.cloudfoundry.operations.applications.DefaultApplications.pushManifest(DefaultApplications.java:432) Error has been observed at the following site(s): |_ Mono.checkpoint ⇢ at org.cloudfoundry.operations.applications.DefaultApplications.pushManifest(DefaultApplications.java:432) |_ Mono.checkpoint ⇢ at org.cloudfoundry.operations.applications.DefaultApplications.push(DefaultApplications.java:404) Stack trace: at org.cloudfoundry.operations.applications.DefaultApplications.lambda$getApplicationId$67(DefaultApplications.java:793)ations.java:793)

So i have changed this line to insert an empty map, if the jsons env null and now it works like expected.

dmikusa commented 3 years ago

Thanks for the additional info. I think I can see how it might get into that case. I'm kind of surprised that the server is returning null and not an empty map, but we should handle it either way.

Two points of feedback on the PR:

  1. Can you add a unit test? Probably here. Just to reproduce the error and then verify it's fixed.
  2. Could you use Optional instead of the ternary operation & != null check? Not a major difference, but it matches the style of the rest of the code better.

Thanks

marc-schaefers commented 2 years ago

Hi @dmikusa-pivotal , the source code is updated now and i added a new push test case.

dmikusa commented 2 years ago

Sorry for the delay, Log4j2 issues had us scrambling before the holidays. Thanks for the PR!