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.

Sign up for GitHub

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

Jump to bottom

as: Sync normalized payload data model and validation to the latest schema #6154

Merged
merged 3 commits into from
Apr 14, 2023
Merged

as: Sync normalized payload data model and validation to the latest schema #6154

merged 3 commits into from
Apr 14, 2023

Conversation

pablojimpas
Copy link
Contributor

@pablojimpas pablojimpas commented Apr 13, 2023 •
edited
Loading

Summary

References TheThingsNetwork/lorawan-devices#395

While responding to a TTN forum thread, I've discovered that the current version of TTS does not support all the normalized payload properties that are present in the schema at this moment. This PR updates the data model and the validation code in TTS to keep in sync with the current JSON schema.

Changes

  • Add CO2 and LightIntensity properties to the Air object.
  • Add Soil object with all of its properties.

Testing

  • Unit tests
Regressions
  • None

Notes for Reviewers

  • Be aware that keeping the schema in sync with the code it's a very repetitive task, so be on the hunt for potential bugs. In the latest commit 92baeb3 I've started abstracting some validation logic into separate functions to avoid potential mismatches in the future, please let me know if you like this approach.

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

johanstokking
johanstokking approved these changes Apr 14, 2023
View reviewed changes
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Great, thanks

@johanstokking johanstokking self-assigned this Apr 14, 2023
@johanstokking johanstokking added this to the v3.25.2 milestone Apr 14, 2023
@johanstokking johanstokking added the c/application server This is related to the Application Server label Apr 14, 2023
@johanstokking
Copy link
Member

CI is broken, will make sure this lands in 3.25.2.

@johanstokking
Copy link
Member

@pablojimpas can you rebase on latest v3.25 branch? CI should work again

@pablojimpas
Copy link
Contributor Author

@pablojimpas can you rebase on latest v3.25 branch? CI should work again

Done! Let's see if it works now.

@adriansmares
Copy link
Contributor

adriansmares commented Apr 14, 2023 •
edited
Loading

I am not sure if the rebase was entirely correct - I would've have expected to see only your original 3 commits. Could you try to reset --hard on top of the latest v3.25 and then cherry pick 1dc5430 , a72038e and 92baeb3 ?

Edit: Seems the syntax is not ok anyway https://github.com/TheThingsNetwork/lorawan-stack/actions/runs/4700691397/workflow . I will check again.

@adriansmares adriansmares mentioned this pull request Apr 14, 2023
Merged
5 tasks
@pablojimpas pablojimpas force-pushed the fix/sync-normalizedpayload-validation-to-schema branch from a06361e to 855b3ba Compare April 14, 2023 14:30
@pablojimpas
Copy link
Contributor Author

Sorry @adriansmares, it's fixed now. I will rebase again once the CI issue is solved.

@adriansmares
Copy link
Contributor

Sorry @adriansmares, it's fixed now. I will rebase again once the CI issue is solved.

Great ! I have merged #6156 - let's see if CI is happier now, after you rebase again.

@pablojimpas
as: Add CO2 and LightIntensity
08364bd 
Sync Air's data model and validation with the latest JSON schema
@pablojimpas
as: Add Soil data model and validation
9fc8242
@pablojimpas
as: Abstract percentages and concentration parsing
76a4ecd 
These kind of of validation checks are so common and might appear even
more as the schema grows. In the same way that definitions are used in
the JSON schema for defining these types, here I've abstracted them to
separate functions to facilitate the maintenance down the road.

I've also added some test to increase coverage and explicitly test the
latest additions.
@pablojimpas pablojimpas force-pushed the fix/sync-normalizedpayload-validation-to-schema branch from 855b3ba to 76a4ecd Compare April 14, 2023 14:52
@johanstokking johanstokking merged commit 6215743 into TheThingsNetwork:v3.25 Apr 14, 2023
@pablojimpas pablojimpas deleted the fix/sync-normalizedpayload-validation-to-schema branch April 14, 2023 17:29
pablojimpas added a commit to pablojimpas/lorawan-stack that referenced this pull request Apr 18, 2023
@pablojimpas
console: Add Soil object to event view.
e773f70 
In TheThingsNetwork#6154 the new Soil object from the normalized payload schema was
added to the Application Server. This commit shows that new object in
the event view of the Console.
@pablojimpas pablojimpas mentioned this pull request Apr 18, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanstokking johanstokking johanstokking approved these changes

@adriansmares adriansmares Awaiting requested review from adriansmares

@KrishnaIyer KrishnaIyer Awaiting requested review from KrishnaIyer KrishnaIyer is a code owner

Assignees

@johanstokking johanstokking

Labels
c/application server This is related to the Application Server
Projects
None yet
Milestone
v3.25.2
Development

Successfully merging this pull request may close these issues.
3 participants
@pablojimpas @johanstokking @adriansmares