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

chore: add FieldMask type to apiutils #5247

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

maxrussell
Copy link
Contributor

Description

While reviewing #5177, I noticed some ergonomic issues working with FieldMasks and wanted to help make that easier. In particular:

  • Doing repeated O(n) scans through a slice of fields to check whether fields were specified in the FieldMask is slow and verbose.
  • An unspecified FieldMask, by convention, should be interpreted by the server as though all fields were specified, so a check was necessary that was easy to miss.
  • FieldMask allows a request to specify an ancestor field such as a.b and should carry the semantics that a descendent field like a.b.c should be considered in the field set.

So I made a new type (with tests) that uses a map for containment checks and respects the listed FieldMask conventions.

Test Plan

No behavior should change as a result of this PR.

@netlify
Copy link

netlify bot commented Oct 12, 2022

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit f2420a0
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/634758b87d87e700086142f5

@netlify
Copy link

netlify bot commented Oct 12, 2022

Deploy Preview for storybook-det canceled.

Name Link
🔨 Latest commit f2420a0
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/634758b8d01682000939ad93

fields := strings.Split(field, ".")
if len(fields) > 1 {
// Continually re-slice the slice of fields to check if an ancestor of field was specified.
for fields = fields[:len(fields)-1]; len(fields) > 0; fields = fields[:len(fields)-1] {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@maxrussell maxrussell merged commit e4b1494 into determined-ai:master Oct 21, 2022
@maxrussell maxrussell deleted the field-mask-type branch October 21, 2022 21:07
@dannysauer dannysauer added this to the 0.19.7 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants