Migrate to new validation plugin #194

rvolosatovs · 2019-02-26T15:08:11Z

Summary:

Closes #69
Closes #51
Closes #260
References #245
References TheThingsIndustries/protoc-gen-fieldmask#22 TheThingsIndustries/protoc-gen-fieldmask#24 TheThingsIndustries/protoc-gen-fieldmask#25 TheThingsIndustries/protoc-gen-fieldmask#26

Changes:

Migrate to new validation plugin
https://github.com/TheThingsIndustries/docker-protobuf/compare/v3.0.24..v3.1.0
Remove Create.* helpers from NS and JS and specify fieldpaths to set explicitly
Rewrite application queue tests in NS according to Harmonize testing of RPC servers #42 (to fix the issue above)
Enable validation of entities in registries
Require App/Dev IDs to be set in JS Redis registry
Add (most of) missing field validations(Add missing field validations #245)
Fix invalid field validations
Fix invalid MAC command encoder/decoder validations(I can extract this in a separate PR if you want)

pkg/rpcmiddleware/validator/validator.go

pkg/rpcmiddleware/validator/validator_test.go

htdvisser · 2019-02-28T15:02:15Z

pkg/ttnpb/application.pb.paths.fm.go

+	"application_ids",
+	"field_mask",
+}
+var ListApplicationsRequestFieldPathsNested = []string{


Inconsistent spacing, but that can be fixed in the future.

TheThingsIndustries/protoc-gen-fieldmask#27

htdvisser · 2019-02-28T15:03:55Z

pkg/ttnpb/application.pb.validate.go

+		switch name {
+		case "ids":
+
+			if v, ok := interface{}(&m.ApplicationIdentifiers).(interface{ ValidateFields(...string) error }); ok {


This looks pretty dirty and I'm not sure the compiler optimizes this, but we can leave it for a future PR.

IDEM, see #194 (comment)

htdvisser · 2019-02-28T15:08:04Z

pkg/ttnpb/application.pb.validate.go

+		cause)
+}
+
+var _ error = ApplicationsValidationError{}


These kind of assertions belong in test code (same below). For every XXXValidationError we initialize two, each taking up 32 bytes of memory (or so). Do we really need to generate them? Can't we just add a unit test in the generator to make sure that (any) generated XXXValidationError implements these?

IDEM, see #194 (comment)

htdvisser

Looks good to me now, but let's test CLI and e2e join/uplink/downlink flow before merging.

rvolosatovs · 2019-02-28T15:29:04Z

~~@adriansmares can you test these changes ?~~
This is not as simple as it seems and won't work directly as it is now

rvolosatovs · 2019-03-05T23:22:49Z

Giving it another thought, it only makes sense to have a ValidateFields(...string) error(or Validate(...string) error on proto message types.
Should we support both Validate() error and ValidateFields(...string) error signatures?
What do you think about this:
The Validate() error would be generated for non-message types, same as it's done by all other validator generators, however ValidateFields(...string) error would only be generated for message types.
The generated validators would assert Validate() error for non-message and non-custom types, in all other cases they would first try ValidateFields(...string) error and then Validate() error in a type switch

In fact, for any non-generated go struct it does not make sense to accept fieldpaths as parameters, e.g.:

lorawan-stack/pkg/frequencyplans/frequencyplans.go

Line 383 in 45ac2ea

func (fp FrequencyPlan) Validate() error {

Since there are no proto paths defined for the struct.
In result we just end up with a confusing method signature and additional error handling for all these cases, where fieldpath cannot be specified.

@johanstokking @htdvisser

johanstokking · 2019-03-06T07:30:40Z

This would mean that ValidateFields() comes instead of Validate() in the interceptor?

I think this could come in handy indeed. I would also see this useful in the registry, where we do want to validate (#70) but where we don't have the means to do so. Sure, with our proto stores we have access to the entire entity, but we don't when working with IoT platform device registries.

rvolosatovs · 2019-03-06T09:55:54Z

Not "instead". The proposal is this ordering - https://github.com/rvolosatovs/lorawan-stack/blob/a761c089c3d176d85bdb9783423d46afbd5707eb/pkg/rpcmiddleware/validator/validator.go#L91-L112

The validator plugin generates ValidateFields(...string) error only for entities, for which fieldpaths can be passed(proto message types), for everything else it generates Validate() error. That means that validators for non-proto types are also Validate() error
In fact message types could still have Validate() error, which would simply be msg.ValidateFields(), since by default we validate all fields

htdvisser · 2019-03-06T10:45:56Z

I think we should not change the interceptor (so it still calls ValidateContext(ctx) error or Validate() error if the former doesn't exist).

On all request messages we implement (by hand) a ValidateContext(ctx) that calls ValidateFields(...) error with the appropriate field mask paths if needed. For instance, on UpdateXXXRequest messages it calls msg.XXX.ValidateFields(msg.FieldMask.Paths...), but on GetXXXRequest messages we can ignore the field mask and only validate the fields of GetXXXRequest.

No need to change what is generated.

rvolosatovs · 2019-03-06T10:55:31Z

I think that works. In registries we then simply call ValidateFields directly.

rvolosatovs · 2019-03-06T11:20:45Z

I made the changes, please take a look.
We still need to support ValidateFields in interceptor, since for the messages not containing the fieldmask we need to validate all fields anyway and they don't have Validate or ValidateContext defined

htdvisser · 2019-03-06T11:24:15Z

LGTM.

Next is to implement Validate/ValidateContext on UpdateXXXRequest and similar messages and to test everything.

- Use `ValidateFields(...string) error` - Enable validation of entities in registries

rvolosatovs · 2019-03-15T11:59:05Z

@adriansmares Can you test this?
I tried OTAA join/uplink/downlink with Class C, but more thorough testing could be useful here

johanstokking

Wait for regression testing confirmation by @adriansmares

adriansmares

Approved. Both OTAA and ABP tests are working correctly.

rvolosatovs added the compat/api This could affect API compatibility label Feb 26, 2019

rvolosatovs added this to the February 2019 milestone Feb 26, 2019

rvolosatovs self-assigned this Feb 26, 2019

rvolosatovs requested a review from KrishnaIyer February 26, 2019 15:08

rvolosatovs force-pushed the feature/51-validation branch 2 times, most recently from 2b57a22 to eac2172 Compare February 26, 2019 17:53

rvolosatovs requested review from johanstokking and removed request for KrishnaIyer February 28, 2019 12:59

johanstokking requested review from htdvisser and removed request for johanstokking February 28, 2019 13:03

htdvisser reviewed Feb 28, 2019

View reviewed changes

rvolosatovs force-pushed the feature/51-validation branch from c402597 to 0c411af Compare February 28, 2019 15:24

rvolosatovs mentioned this pull request Feb 28, 2019

Inconsistent spacing in PathBuilder module TheThingsIndustries/protoc-gen-fieldmask#27

Open

htdvisser approved these changes Feb 28, 2019

View reviewed changes

htdvisser requested a review from adriansmares February 28, 2019 15:28

htdvisser modified the milestones: February 2019, March 2019 Feb 28, 2019

rvolosatovs force-pushed the feature/51-validation branch from 0c411af to 712efad Compare March 5, 2019 23:04

rvolosatovs requested a review from htdvisser March 5, 2019 23:05

rvolosatovs added the needs/discussion We need to discuss this label Mar 5, 2019

rvolosatovs force-pushed the feature/51-validation branch from 712efad to 97f87aa Compare March 6, 2019 11:18

rvolosatovs added the blocking Another issue or pull request is waiting for this label Mar 6, 2019

Roman Volosatovs and others added 18 commits March 15, 2019 12:10

dev: Vendor validator dependencies

1ac7663

all: Migrate to new validation method signature

833ddeb

- Use `ValidateFields(...string) error` - Enable validation of entities in registries

util: Support ValidateFields in validator middleware

2faaa36

util: Fix PingSlotChannel MAC command frequency encoding

3f214ab

util: Fix crypto package

0c52e16

ns: Fix panics in application queue

26a2358

ns: Harmonize tests, adapt to message validation

1f559d4

js: Harmonize tests, adapt to message validation

8d91552

js: Ensure App/Dev IDs are set in registries

5a8a37b

ns: Fix panic on message drops

e899ff5

ns: Fix BeaconFreqReq MAC encoding

a82632f

ns: Adapt to API changes

f1abfd4

js: Make device primary key uid and secondary key EUIs

7f5ab3e

ns: Fix argument order

2c7fa8d

js: Make SetByEUI atomic in Redis

240601e

util: Do not change given uplink message when getting identifiers

97789d2

util: Adapt to API changes

1df8842

dev: Regenerate messages

6c170e1

rvolosatovs force-pushed the feature/51-validation branch from 604fdd1 to 6c170e1 Compare March 15, 2019 11:14

rvolosatovs requested a review from johanstokking March 15, 2019 11:15

Roman Volosatovs added 2 commits March 15, 2019 12:49

ns: Fix decoding of uplinks

1a04640

js: Remove redundant checks

7d0d128

rvolosatovs force-pushed the feature/51-validation branch from e599a3c to 7d0d128 Compare March 15, 2019 11:50

johanstokking approved these changes Mar 15, 2019

View reviewed changes

adriansmares approved these changes Mar 15, 2019

View reviewed changes

rvolosatovs merged commit 126017d into master Mar 15, 2019

rvolosatovs deleted the feature/51-validation branch March 15, 2019 13:40

rvolosatovs mentioned this pull request Mar 22, 2019

Identity Server (and some other) field validations #341

Merged

htdvisser mentioned this pull request Apr 8, 2019

Wrap validation errors #493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to new validation plugin #194

Migrate to new validation plugin #194

rvolosatovs commented Feb 26, 2019 •

edited by johanstokking

Loading

htdvisser Feb 28, 2019

rvolosatovs Feb 28, 2019

htdvisser Feb 28, 2019

rvolosatovs Feb 28, 2019

htdvisser Feb 28, 2019

rvolosatovs Feb 28, 2019

htdvisser left a comment

rvolosatovs commented Feb 28, 2019 •

edited

Loading

rvolosatovs commented Mar 5, 2019 •

edited

Loading

johanstokking commented Mar 6, 2019 •

edited

Loading

rvolosatovs commented Mar 6, 2019 •

edited

Loading

htdvisser commented Mar 6, 2019 •

edited

Loading

rvolosatovs commented Mar 6, 2019

rvolosatovs commented Mar 6, 2019

htdvisser commented Mar 6, 2019

rvolosatovs commented Mar 15, 2019

johanstokking left a comment

adriansmares left a comment

Migrate to new validation plugin #194

Migrate to new validation plugin #194

Conversation

rvolosatovs commented Feb 26, 2019 • edited by johanstokking Loading

htdvisser Feb 28, 2019

Choose a reason for hiding this comment

rvolosatovs Feb 28, 2019

Choose a reason for hiding this comment

htdvisser Feb 28, 2019

Choose a reason for hiding this comment

rvolosatovs Feb 28, 2019

Choose a reason for hiding this comment

htdvisser Feb 28, 2019

Choose a reason for hiding this comment

rvolosatovs Feb 28, 2019

Choose a reason for hiding this comment

htdvisser left a comment

Choose a reason for hiding this comment

rvolosatovs commented Feb 28, 2019 • edited Loading

rvolosatovs commented Mar 5, 2019 • edited Loading

johanstokking commented Mar 6, 2019 • edited Loading

rvolosatovs commented Mar 6, 2019 • edited Loading

htdvisser commented Mar 6, 2019 • edited Loading

rvolosatovs commented Mar 6, 2019

rvolosatovs commented Mar 6, 2019

htdvisser commented Mar 6, 2019

rvolosatovs commented Mar 15, 2019

johanstokking left a comment

Choose a reason for hiding this comment

adriansmares left a comment

Choose a reason for hiding this comment

rvolosatovs commented Feb 26, 2019 •

edited by johanstokking

Loading

rvolosatovs commented Feb 28, 2019 •

edited

Loading

rvolosatovs commented Mar 5, 2019 •

edited

Loading

johanstokking commented Mar 6, 2019 •

edited

Loading

rvolosatovs commented Mar 6, 2019 •

edited

Loading

htdvisser commented Mar 6, 2019 •

edited

Loading