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

Harmonize field mask validation #52

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cvetkovski98
Copy link
Member

@cvetkovski98 cvetkovski98 commented Jan 13, 2025

Summary

This PR extends the functionality of the field mask generation by generating the field mask validation maps based on a newly introduced rpcmask RPC method option.

References TheThingsIndustries/lorawan-stack#4176

Changes

  • Use buf to build and lint proto files
  • Introduce an RPC method option to generate field mask validation
  • Update the code generator to recognize and use the new message option

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@cvetkovski98 cvetkovski98 force-pushed the feature/fieldmask-validation branch from 165b208 to c0e51ef Compare January 13, 2025 20:16
@cvetkovski98 cvetkovski98 marked this pull request as ready for review January 13, 2025 20:40
@cvetkovski98 cvetkovski98 self-assigned this Jan 13, 2025
@KrishnaIyer KrishnaIyer added this to the v3.33.1 milestone Jan 14, 2025
@KrishnaIyer KrishnaIyer self-requested a review January 14, 2025 13:13
Copy link

@halimi halimi left a comment

Choose a reason for hiding this comment

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

The code changes looks good to me. As far as I understand the referenced issue, this can be a solution but I cannot fully judge.

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

Why did you choose to add this as an option to the message? I suppose it would be relevant for request messages, right? Why not an rpc option?

@cvetkovski98
Copy link
Member Author

I initially though that it made the implementation a bit more straightforward, but indeed, I realize it makes more sense to be an RPC method option. I moved it now.

continue
}
rpcFieldMaskPaths[rpcMethodIdentifier] = RPCFieldMaskPathValue{
All: fmt.Sprintf("%sFieldPathsNested", rpcmask.GetMessageName()),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having to pass the message name through the options, which is error prone as it may cause conflicts with other rpcs that use the same message but with different field masks, would it be an idea to construct a unique name from the service name + method name by concatenating it?

Copy link
Member Author

@cvetkovski98 cvetkovski98 Jan 16, 2025

Choose a reason for hiding this comment

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

The logic preceding this block generates all possible nested paths for the proto message. The message name in the options is used to reference the complete set of nested paths (e.g.,EndDeviceFieldPathsNested). Even if the same field mask logic is repeated across different RPCs, there’s no conflict, as all references point to the same generated variable for the respective message.

The unique name you suggest (constructed from the service name + method name) is already used to associate the generated RPC mask (allowed fields) with the correct RPC (package + service name + method).

To address the error-prone passing of the message name, I could explore using reflection to extract the message name from the first RPC request parameter. However, this would require developers to implicitly understand that the first argument is used for generating the RPC mask. From what I’ve observed in the End Device registries, the field mask typically applies to the first argument (the request message). The previous solution (embedding RPC masks in the MessageOptions) didn’t require passing the message name, but it relied on the fully qualified method name, which can also introduce error-proneness.

Copy link
Member

Choose a reason for hiding this comment

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

Even if the same field mask logic is repeated across different RPCs, there’s no conflict, as all references point to the same generated variable for the respective message.

I mean, in cases where the field mask is different while using the same request message type.

Basically, why is the message name relevant? The allowed field mask applies to the rpc and the field mask passed to it. The developer should validate the request message with the generated allowed field mask paths.

@KrishnaIyer KrishnaIyer modified the milestones: v3.33.1, v3.34.0 Feb 3, 2025
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.

4 participants