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

Add missing field validations #245

Closed
rvolosatovs opened this issue Mar 6, 2019 · 4 comments · Fixed by #341
Closed

Add missing field validations #245

rvolosatovs opened this issue Mar 6, 2019 · 4 comments · Fixed by #341
Assignees
Labels
c/shared This is shared between components compat/api This could affect API compatibility in progress We're working on it
Milestone

Comments

@rvolosatovs
Copy link
Contributor

rvolosatovs commented Mar 6, 2019

Summary:

Currently there is a bunch of fields in our API, which lack validation options.
E.g.

// LoRaWAN MAC version.
MACVersion lorawan_version = 2 [(gogoproto.customname) = "LoRaWANVersion"];
// LoRaWAN PHY version.
PHYVersion lorawan_phy_version = 3 [(gogoproto.customname) = "LoRaWANPHYVersion"];

Refs #51 #69

Why do we need this?

To make sure we don't store garbage

What is already there? What do you see now?

Possible garbage in fields

What is missing? What do you want to see?

Validation

How do you propose to implement this?

Add the validators.
For the exact case linked I propose (validate.rules).enum.defined_only = true
and (validate.rules).enum.not_in = [0]
See https://github.com/lyft/protoc-gen-validate#enums

@rvolosatovs rvolosatovs added this to the March 2019 milestone Mar 6, 2019
@rvolosatovs rvolosatovs self-assigned this Mar 6, 2019
@rvolosatovs rvolosatovs added c/shared This is shared between components compat/api This could affect API compatibility labels Mar 6, 2019
@rvolosatovs rvolosatovs added the blocked This can't continue until another issue or pull request is done label Mar 6, 2019
@rvolosatovs
Copy link
Contributor Author

Blocked by #194

@johanstokking johanstokking added the in progress We're working on it label Mar 12, 2019
@rvolosatovs
Copy link
Contributor Author

I added a bunch of missing validations in #194, however those were mostly NS or JS-related and obvious ones, like email.
Let's go component-by-component and ensure that all the fields that need validation are validated.
NS and JS should be fine, however we should still look through GS, AS and IS. I'll assign package maintainers.
@johanstokking please check AS and GS-related messages
@htdvisser please check IS-related messages

@htdvisser htdvisser removed the blocked This can't continue until another issue or pull request is done label Mar 18, 2019
@htdvisser
Copy link
Contributor

Unblocked since #194 was merged

@johanstokking
Copy link
Member

@rvolosatovs checked both, only addition necessary seems to be in #297

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/shared This is shared between components compat/api This could affect API compatibility in progress We're working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants