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

Update unmarshalNBytes to return nil when len of value is zero #7088

Merged
merged 2 commits into from
May 22, 2024

Conversation

nicholaspcr
Copy link
Contributor

Summary

References #7087

Where a JSON object with null as a value generated a EUI filled with zeros instead of it being nil.

Changes

  • return nil instead of a slice with n zero value elements

Testing

A bit complicated to validate the changes introduced by this PR besides running the current unit and e2e tests.
Reason being that the method is used in a lot of the API definitions, making it hard to manually test it.

Regressions

This is a extremely small PR but quite big in terms of impact, this method is used in unmarshalling any EUI field in the API, so if one of the routes doesn't handle a nil value in the field instead of a slice filled with empty values then it will generate an error.

Fortunately most places where we process any EUI field we use MustEUI64 followed by OrZero and places without the OrZero there is a explicit check for the field value beforehand.

But it is used in many places and it is possible that something was missed, making it so that its worth paying extra attention to the deployment of this version.

Notes for Reviewers

Mostly what is written on the Regressions section.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@nicholaspcr nicholaspcr added this to the v3.30.2 milestone May 19, 2024
@nicholaspcr nicholaspcr self-assigned this May 19, 2024
@nicholaspcr nicholaspcr force-pushed the fix/7087-eui-null-conversion-to-zeros-string branch from 5046600 to 7c46115 Compare May 19, 2024 19:05
@nicholaspcr nicholaspcr marked this pull request as ready for review May 21, 2024 11:44
@nicholaspcr nicholaspcr requested a review from a team as a code owner May 21, 2024 11:44
@nicholaspcr nicholaspcr requested a review from johanstokking May 21, 2024 11:44
@adriansmares adriansmares merged commit bda98a8 into v3.30 May 22, 2024
13 checks passed
@adriansmares adriansmares deleted the fix/7087-eui-null-conversion-to-zeros-string branch May 22, 2024 10:56
@KrishnaIyer
Copy link
Member

@nicholaspcr: Can you please add concrete steps for testing including regressions? The current description is pretty vague.

@nicholaspcr
Copy link
Contributor Author

I already validated on staging but below are the steps that I used:

test steps
# Create a gateway on staging via http Request
# Example used to test it on staging

## Testing the 
curl -v -X POST \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer <token>" \
  -d '{ "gateway": { "ids": { "gateway_id": "<gtw_test_name>", "eui": null }, "name": "", "frequency_plan_ids": [ "EU_863_870_TTN" ], "require_authenticated_connection": false, "status_public": true, "location_public": true, "gateway_server_address": "localhost", "enforce_duty_cycle": true, "schedule_anytime_delay": "0.530s" } }' \
  "https://<staging_url>/api/v3/users/nicholas-cristofaro/gateways"

### Get the gateway via the CLI/Console/HTTP-request
### The `gateway.ids.eui` should be empty, meaning that it should not be returned. Before the changes in the PR it would have been zeros.

## Testing the update operation for the sake of completion

### Update gateway to include a random EUI

curl -X PUT \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer <token>" \
  -d '{ "gateway": { "ids": { "eui": "8811111112211101" }}, "field_mask": { "paths": [ "ids.eui" ] } }' \
  "https://<staging_url>/api/v3/gateways/<gtw_test_name>"

#### Get the gateway via the CLI/Console/HTTP-request
#### The `gateway.ids.eui` should be the set value.

### Testing the setting it explicitly to `null` via update.

curl -X PUT \
  -H "Content-Type: application/json" \
  -H "Authorization: Bearer <token>" \
  -d '{ "gateway": { "ids": { "eui": null }}, "field_mask": { "paths": [ "ids.eui" ] } }' \
  "https://<staging_url>/api/v3/gateways/<gtw_test_name>"

### Get the gateway via the CLI/Console/HTTP-request
### The `gateway.ids.eui` should be empty, meaning that it should not be returned. Before the changes in the PR it would have been zeros.

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.

3 participants