Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

fix ContextBuilder.copyFrom NPE #15

Merged
merged 2 commits into from
Nov 9, 2023
Merged

fix ContextBuilder.copyFrom NPE #15

merged 2 commits into from
Nov 9, 2023

Conversation

pedroafonsodias
Copy link
Contributor

base copyOnWrite*Attributes on respective objects nullability otherwise they will not be initialized causing NPE

@pedroafonsodias
Update ContextBuilder.java
ddba70a 
base copyOnWrite*Attributes on respective objects nullability otherwise they will not be initialized causing NPE
@pedroafonsodias pedroafonsodias requested a review from a team November 9, 2023 15:31
driverpt
driverpt reviewed Nov 9, 2023
View reviewed changes
src/main/java/com/launchdarkly/sdk/ContextBuilder.java
@@ -380,8 +380,8 @@ ContextBuilder copyFrom(LDContext context) {
anonymous = context.isAnonymous();
attributes = context.attributes;
privateAttributes = context.privateAttributes;
copyOnWriteAttributes = true;
copyOnWritePrivateAttributes = true;
copyOnWriteAttributes = context.attributes != null;
Copy link

Choose a reason for hiding this comment

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

Suggestion: Objects.nonNull(context.attributes)

@tanderson-ld
Copy link
Contributor

@pedroafonsodias, thank you for bringing this to our attention. Could you provide steps/pseudocode for reproducing the NPE.

@pedroafonsodias
Copy link
Contributor Author 

LDContext customContext = LDContext.builderFromContext(ldContext)
      .set("some_attribute", someValue)
      .build();
    ldClient.identify(customContext);

    @Bean
    public LDContext ldContext(final LaunchDarklyProperties properties) {
      return LDContext.builder(properties.getClientSideId()).build();
    }

Using springboot to create a default context and when reusing it to customize a new context it will fail unless we specify a dummy attribute when creating the initial LDContext.

I have added a test in the mean time.

@tanderson-ld
Copy link
Contributor

@pedroafonsodias, approved but going to take a bit more time making sure there are no adverse consequences that others may have already been accounting for in production. Thank you for your contribution!

@tanderson-ld tanderson-ld changed the base branch from main to main-contrib November 9, 2023 21:56
@tanderson-ld tanderson-ld merged commit bc12d19 into launchdarkly:main-contrib Nov 9, 2023
@driverpt
Copy link

Any ETA when this is going to be released?

@tanderson-ld
Copy link
Contributor

Working on releasing today.

@LaunchDarklyReleaseBot LaunchDarklyReleaseBot mentioned this pull request Nov 13, 2023
Merged
LaunchDarklyReleaseBot added a commit that referenced this pull request Nov 13, 2023
@LaunchDarklyReleaseBot @pedroafonsodias @tanderson-ld
prepare 2.1.1 release (#16)
285c658 
## [2.1.1] - 2023-11-13
### Fixed:
- Fixes NPE when interacting with Context created by use of `copyFrom`.
(Thanks, [

pedroafonsodias](#15))

---------

Co-authored-by: Pedro Dias <pedroafonsodias@gmail.com>
Co-authored-by: Todd Anderson <127344469+tanderson-ld@users.noreply.github.com>
@tanderson-ld
Copy link
Contributor

tanderson-ld commented Nov 13, 2023 •
edited
Loading

@pedroafonsodias and @driverpt , what versions of the Java Server SDK are you using?

@LaunchDarklyReleaseBot LaunchDarklyReleaseBot mentioned this pull request Nov 13, 2023
Merged
@driverpt
Copy link

driverpt commented Nov 13, 2023 •
edited
Loading

5.10, very old versions. We detected this while upgrading to the latest version

LaunchDarklyReleaseBot added a commit to launchdarkly/java-sdk-internal that referenced this pull request Nov 14, 2023
@LaunchDarklyReleaseBot @eli-darkly
@tanderson-ld @ld-repository-standards @kparkinson-ld @kinyoklion
prepare 1.2.1 release (#9)
8bb6c02 
## [1.2.1] - 2023-11-13
### Fixed:
- Fixes NPE when interacting with Context created by use of `copyFrom`.
(Thanks, [

pedroafonsodias](launchdarkly/java-sdk-common#15))

---------

Co-authored-by: Eli Bishop <eli@launchdarkly.com>
Co-authored-by: LaunchDarklyReleaseBot <launchdarklyreleasebot@launchdarkly.com>
Co-authored-by: Todd Anderson <tanderson@launchdarkly.com>
Co-authored-by: tanderson-ld <127344469+tanderson-ld@users.noreply.github.com>
Co-authored-by: ld-repository-standards[bot] <113625520+ld-repository-standards[bot]@users.noreply.github.com>
Co-authored-by: Kane Parkinson <93555788+kparkinson-ld@users.noreply.github.com>
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
@LaunchDarklyReleaseBot LaunchDarklyReleaseBot mentioned this pull request Nov 14, 2023
Merged
LaunchDarklyReleaseBot added a commit to launchdarkly/java-server-sdk that referenced this pull request Nov 14, 2023
@LaunchDarklyReleaseBot @eli-darkly
@aengelberg @antonmos @LaunchDarklyCI @gwhelanLD @ssrm @hroederld @bwoskow-ld @apache-hb @robertjneal @samstokes @ember-stevens @louis-launchdarkly @tanderson-ld @keelerm84 @ld-repository-standards @kparkinson-ld @kinyoklion
prepare 7.1.1 release (#319)
5a69a6a 
## [7.1.1] - 2023-11-14
### Fixed:
- Fixes NPE when interacting with Context created by copying. (Thanks, [

pedroafonsodias](launchdarkly/java-sdk-common#15))

---------

Co-authored-by: Eli Bishop <eli@launchdarkly.com>
Co-authored-by: LaunchDarklyReleaseBot <launchdarklyreleasebot@launchdarkly.com>
Co-authored-by: Alex Engelberg <aengelberg@launchdarkly.com>
Co-authored-by: Anton Mostovoy <anton@mostovoy.net>
Co-authored-by: LaunchDarklyCI <dev@launchdarkly.com>
Co-authored-by: LaunchDarklyCI <LaunchDarklyCI@users.noreply.github.com>
Co-authored-by: Gavin Whelan <gwhelan@launchdarkly.com>
Co-authored-by: ssrm <ssrm@users.noreply.github.com>
Co-authored-by: Harpo Roeder <hroeder@launchdarkly.com>
Co-authored-by: Ben Woskow <48036130+bwoskow-ld@users.noreply.github.com>
Co-authored-by: Elliot <35050275+Apache-HB@users.noreply.github.com>
Co-authored-by: Robert J. Neal <rneal@launchdarkly.com>
Co-authored-by: Robert J. Neal <robertjneal@users.noreply.github.com>
Co-authored-by: Sam Stokes <sstokes@launchdarkly.com>
Co-authored-by: Ember Stevens <ember.stevens@launchdarkly.com>
Co-authored-by: ember-stevens <79482775+ember-stevens@users.noreply.github.com>
Co-authored-by: Alex Engelberg <alex.benjamin.engelberg@gmail.com>
Co-authored-by: Louis Chan <lchan@launchdarkly.com>
Co-authored-by: Louis Chan <91093020+louis-launchdarkly@users.noreply.github.com>
Co-authored-by: Todd Anderson <tanderson@launchdarkly.com>
Co-authored-by: tanderson-ld <127344469+tanderson-ld@users.noreply.github.com>
Co-authored-by: Matthew M. Keeler <mkeeler@launchdarkly.com>
Co-authored-by: ld-repository-standards[bot] <113625520+ld-repository-standards[bot]@users.noreply.github.com>
Co-authored-by: Kane Parkinson <93555788+kparkinson-ld@users.noreply.github.com>
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
@tanderson-ld
Copy link
Contributor

tanderson-ld commented Nov 14, 2023 •
edited
Loading

Should now be available in Java Server SDK 7.1.1. May still be propagating through package managers.

@tanderson-ld tanderson-ld mentioned this pull request Mar 7, 2024
Merged
tanderson-ld added a commit to launchdarkly/android-client-sdk that referenced this pull request Mar 7, 2024
@tanderson-ld
fix: propagating NPE fix for ContextBuilder.fromContext bug (#246)
3205005 
**Related issues**

This fix never propagated into the Android SDK and another customer
reported it. launchdarkly/java-sdk-common#15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@driverpt driverpt driverpt left review comments

@tanderson-ld tanderson-ld tanderson-ld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pedroafonsodias @tanderson-ld @driverpt