Skip to content

Conversation

@choopm
Copy link
Contributor

@choopm choopm commented Nov 5, 2025

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

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2025

CLA assistant check
All committers have signed the CLA.

@choopm choopm force-pushed the main branch 7 times, most recently from 1a19a4b to 3d47abb Compare November 5, 2025 15:53
@choopm choopm changed the title Add fieldmask rule to check for valid/invalid paths Add fieldmask rule to check for accept/reject paths Nov 5, 2025
@choopm
Copy link
Contributor Author

choopm commented Nov 5, 2025

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

@choopm choopm force-pushed the main branch 3 times, most recently from 80dbb4d to 51abb66 Compare November 17, 2025 06:01
Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Few comments to get it consistent with other rules, but there's a detail we might want to address:

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?

@choopm
Copy link
Contributor Author

choopm commented Nov 18, 2025

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 #429 (review):
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.

@timostamm
Copy link
Member

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.

@choopm choopm requested a review from rodaine November 19, 2025 15:14
@choopm choopm changed the title Add fieldmask rule to check for accept/reject paths Add fieldmask rule to check paths in/not_in Nov 19, 2025
@choopm choopm changed the title Add fieldmask rule to check paths in/not_in Add field_mask rules to check paths in/not_in Nov 19, 2025
@rodaine rodaine changed the title Add field_mask rules to check paths in/not_in Add field_mask rules const/in/not_in Nov 19, 2025
@choopm
Copy link
Contributor Author

choopm commented Nov 19, 2025

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

@rodaine
Copy link
Member

rodaine commented Nov 19, 2025

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.

This adds support for the well-known type google.protobuf.FieldMask:
- const: exact matching of paths
- in: accepted paths (violation if unlisted paths or subpaths are encountered)
- not_in: rejected paths (violation if any specified path or subpath is encountered)

Usage examples:

const:
```proto
message MyFieldMask {
  //  The `value` FieldMask must be equal to `const`.
  google.protobuf.FieldMask value = 1 [(buf.validate.field).field_mask.const = {
      paths: ["foo", "bar"]
  }];
}
```

in:
```proto
message MyFieldMask {
  //  The `value` FieldMask must only contain paths listed in `in`.
  google.protobuf.FieldMask value = 1 [(buf.validate.field).field_mask = {
      in: ["a", "b", "c.a"]
  }];
}
```

not_in:
```proto
message MyFieldMask {
  //  The `value` FieldMask must not contain paths listed in `not_in`.
  google.protobuf.FieldMask value = 1 [(buf.validate.field).field_mask = {
      not_in: ["forbidden", "immutable", "c.a"]
  }];
}
```

Closes bufbuild#428.

Signed-off-by: Christoph Hoopmann <[email protected]>
@choopm
Copy link
Contributor Author

choopm commented Nov 19, 2025

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?

@rodaine
Copy link
Member

rodaine commented Nov 19, 2025

Should I push these changes aswell?

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

Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Thanks, @choopm!

@rodaine rodaine merged commit aabae75 into bufbuild:main Nov 19, 2025
6 checks passed
@rodaine rodaine linked an issue Nov 19, 2025 that may be closed by this pull request
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.

[Feature Request] Support google.protobuf.FieldMask well-known type

4 participants