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

fix(AIP-160): move validation guidance to new section #1460

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

noahdietz
Copy link
Collaborator

The only mention of what to do in the face of a non-compliant filter is buried in a somewhat unrelated subsection. Furthermore, no explicit guidance is given as to how schematically invalid filters are handled in general e.g. referring to a field that doesn't exist, using a value that doesn't align with the field's type, etc.

This moves that "non-compliant filter == INVALID_ARGUMENT" to a dedicated Validation section and includes mention of some schematic validation.

Internal bug http://b/383153119

@noahdietz noahdietz requested a review from a team as a code owner December 11, 2024 22:49
@noahdietz noahdietz requested review from neomagus00 and jskeet and removed request for neomagus00 December 11, 2024 22:49

Schematic validation refers, but is not limited to, the following:

- Fields referenced in the `filter` **must** exist on the filtered schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we have a notion of a sort of "logical" field, or do we require the use of functions for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you explain that a bit more? Hwat do you mean by "logical" field?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something that may not end up being exposed as an actual field, but has some sort of logical value. For example, if we had an expiry timestamp, there could be some sort of "expires less than one minute from now" that you might want to filter on. I suspect that in most cases, output fields would be fine, and functions cover other situations, but it's just something to consider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yes I see what you mean. In my mind I refer to those as "virtual" fields. I believe that is what functions are for atm.

aip/general/0160.md Show resolved Hide resolved
aip/general/0160.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Have a think about the logical field part, but it's fine to merge as-is.

@noahdietz noahdietz merged commit aaceb3c into aip-dev:master Dec 17, 2024
2 checks passed
@noahdietz noahdietz deleted the aip-160-validation branch December 17, 2024 17:45
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.

3 participants