Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid NPE in DefaultApplications.java #1126

Merged
merged 3 commits into from
Jan 4, 2022
Merged

Conversation

marc-schaefers
Copy link
Contributor

avoid NPE, if environmentJsons is null from application entity

avoid NPE, if environmentJsons is null from application entity
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 18, 2021

CLA Signed

The committers are authorized under a signed CLA.

@marc-schaefers marc-schaefers changed the title Update DefaultApplications.java Avoid NPE in DefaultApplications.java Nov 18, 2021
@dmikusa
Copy link
Contributor

dmikusa commented Nov 18, 2021

@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.

@dmikusa dmikusa added the bug label Nov 18, 2021
@marc-schaefers
Copy link
Contributor Author

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
Copy link
Contributor

dmikusa commented Nov 21, 2021

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
Copy link
Contributor Author

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

@dmikusa dmikusa merged commit 6e94919 into cloudfoundry:main Jan 4, 2022
@dmikusa
Copy link
Contributor

dmikusa commented Jan 4, 2022

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

dmikusa pushed a commit that referenced this pull request Feb 17, 2022
Avoid NPE, if `environmentJsons` is null from the application entity & add test for push with null entity env.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants