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

Avoid to call registrationStore.removeObservation with null registration ID #1388

Merged
merged 7 commits into from
Jan 26, 2023

Conversation

sbernard31
Copy link
Contributor

This aims to fix #1384

But working on this, I also polish some details which sounds not so good to me :

  • NPE in RedisRegistrationStoreTest
  • lastEndpointUsed in Registration should be mandatory
  • add validation on lastEndpointUsed
  • Remove redundant validation in Registration Constructor
  • Avoid any Registration call with null value for registration ID.

@cyril2maq
Copy link

I did run my tests which led to create this issue with this PR, it is now OK👍

Regarding the last commit, I think an additional test could be interesting. Maybe a unit test on LwM2mObservationStore.remove(Token token) method checking that the observation has been removed.

Suggestion for LwM2mObservationStoreTest :

    @Test
    public void remove_observation() {
        // given
        givenASimpleRegistration(lifetime);
        store.addRegistration(registration);

        org.eclipse.californium.core.observe.Observation observationToStore = prepareCoapCompositeObservation();
        observationStore.put(exampleToken, observationToStore);

        // when
        observationStore.remove(exampleToken);

        // then
        Observation leshanObservation = store.getObservation(registrationId,
                new ObservationIdentifier(exampleToken.getBytes()));
        assertNull(leshanObservation);
    }

@sbernard31
Copy link
Contributor Author

I added your suggestions (commit dbe5aa2)

I guess I can merge the PR in master now ?

@cyril2maq
Copy link

cyril2maq commented Jan 26, 2023

I guess I can merge the PR in master now ?

Yes !

@sbernard31 sbernard31 merged commit dbe5aa2 into master Jan 26, 2023
@sbernard31
Copy link
Contributor Author

Thx for reviewing this 😉

@sbernard31 sbernard31 deleted the fix_redis_store branch January 26, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LwM2mObservationStore 'remove(Token token)' is not functionnal
2 participants