protovalidate icon indicating copy to clipboard operation
protovalidate copied to clipboard

Add fieldmask rule to check for accept/reject paths

Open choopm opened this issue 4 weeks ago • 2 comments

This implements #428. I might require some help regarding the test suites. Let me know what you think.

choopm avatar Nov 05 '25 13:11 choopm

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 05 '25 13:11 CLAassistant

Ready for review. Please pay extra attention to the tests and if the implementation is meeting your expectations.

choopm avatar Nov 05 '25 16:11 choopm

Thank you for the detailled review. I agree on the requested changes and ~will provide these soon~ I have implemented those.

Regarding your suggestion in https://github.com/bufbuild/protovalidate/pull/429#pullrequestreview-3422608072: I have implemented it right away. It makes absolutely more sense to check for subpaths aswell.

~What if we add two more fields:~

~- strict_in~ ~- strict_not_in~

~and change the current fields to act on subpaths aswell according to your suggestion?~

~I am still undecided whether the strict_ variants are of any use though.~

choopm avatar Nov 18 '25 17:11 choopm

A field_mask.in rule with a path of a would hypothetically imply that any sub-path also would be valid, such as a.b and a.c. As written, this limits the field mask to only target all of a and not just some of its sub-paths.

Changing the expression to support matching subpaths isn't terribly difficult (though polynomial time complexity). The expression for in would be:

!this.paths.all(p, p in getField(rules, 'in') || getField(rules, 'in').exists(f, p.startsWith(f+"."))) ? ... : ...

@timostamm, any feelings on this?

Agreed.

I expect a mask paths: ["a"] to include a.b and a.c. It's not specified in field_mask.proto, but that's how it's implemented (example).

IMO it makes sense to apply the same logic for field_mask.in and field_mask.not_in, but it should be documented in the rule comments.

timostamm avatar Nov 19 '25 13:11 timostamm

Are CI errors related to license headers removed when using buf generate? I did not commit these changes on purpose.

edit: well, and I did not regenerate..

choopm avatar Nov 19 '25 15:11 choopm

Looks like bazel is unhappy now. You'll need to add the field mask dep to proto/protovalidate/buf/validate/BUILD.bazel. Gazelle should add it when you run generate, but it looks like you may need to do it manually.

rodaine avatar Nov 19 '25 15:11 rodaine

Done, run generate and pushed. Here is what I'm referring to when I mentioned license headers being removed by generate:

git status

 M tools/internal/gen/buf/validate/conformance/cases/ignore_empty_proto_editions.pb.go
 M tools/internal/gen/buf/validate/conformance/cases/ignore_proto_editions.pb.go
 M tools/internal/gen/buf/validate/conformance/cases/predefined_rules_proto_editions.pb.go
 M tools/internal/gen/buf/validate/conformance/cases/required_field_proto_editions.pb.go

git diff - all the same

@@ -1,17 +1,3 @@
-// Copyright 2023-2025 Buf Technologies, Inc.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//      http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-

Should I push these changes aswell?

choopm avatar Nov 19 '25 15:11 choopm

Should I push these changes aswell?

Nope, there's likely a bug in our Makefile. I'll look into it.

rodaine avatar Nov 19 '25 16:11 rodaine