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

Set up a place for body validations & add the first new check #6145

Closed
wants to merge 1 commit into from

Conversation

dylan-apollo
Copy link
Member

A bunch of refactoring to set us up for body validations—and adds a single check, if you use empty $args with no path but have no arguments defined on the field.

This doesn't use the visitor stuff because we're interested in the opposite info—where's the data coming from, not where is it going to. Left a bunch of TODOs where we should add more validations.

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 11, 2024

✅ Docs Preview Ready

No new or changed pages found.

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

I think the JSONSelection::external_var_paths method (provided by pub(crate) trait ExternalVarPaths) will help this validation code be a little less coupled to the internal PathList structure, and also catch more $args paths that are currently skipped.

Comment on lines +48 to +51
fn validate_path_selection(path_selection: PathSelection, arg: Arg) -> Result<(), Message> {
let PathSelection { path } = path_selection;
match path.as_ref() {
PathList::Var(variable, trailing) => {
Copy link
Member

@benjamn benjamn Oct 16, 2024

Choose a reason for hiding this comment

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

Probably need to process trailing in more than just the $args case, since other $args paths could appear elsewhere along the path, within -> method arguments for example.

Comment on lines +60 to +61
KnownVariable::Dollar => {
// TODO: Validate that this is followed only by a method
Copy link
Member

Choose a reason for hiding this comment

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

We're now recommending/enforcing the $.data syntax for single-key paths, so $ can definitely be followed by a .key as well as a -> method.

pub(super) enum PathList {
pub(crate) enum PathList {
Copy link
Member

Choose a reason for hiding this comment

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

I wish the PathList enum could remain pub(super), because its internal structure needs to change fairly often, and adding more pattern matching against those internal details makes changing them harder.

Have you considered using the ExternalVarPaths trait?

pub(crate) trait ExternalVarPaths {
fn external_var_paths(&self) -> Vec<&PathSelection>;
}

This should give you all the variable paths referring to external inputs (like $this, $args, and $config), fully flattened, so you can easily loop over them and validate the $args paths.

@dylan-apollo
Copy link
Member Author

We're working on a more general interface for iterating over mapping inputs/outputs. We'll use that for validating bodies instead of the AST directly

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