Field Arguments RFC/Spec

[sordina]

This PR updates the NDC-Spec to allow arguments to fields.

See the rendered RFC here.

The motivation for this change is to generalise the API surface to allow more expressive queries - This should be very similar to collection arguments in practice but can apply to any field, not just top-level collections.

Some examples of why this might be useful:

• JSON Operations: For a JSON field in a PG table, several JSON functions could be exposed as fields but they require arguments to be useful. Such as #>
• Vector Operations for LLMs etc.
• Advanced Geo Operations
• GraphQL federation: Forwarding Hasura GQL schemas over NDC will require this change if we want to expose root fields as commands instead of collections.
• etc.

Changes

The following changes are present in this PR:

• An RFC has been added detailing this change
• Documentation in the Spec has been updated detailing this functionality
• Schema datatypes now support defining field arguments in schemas
  • Main change: ObjectField in ndc-models/src/lib.rs
  • Copying pattern from CollectionInfo of BTreeMap<String, ArgumentInfo>
  • This field is optional for backwards compatibility
• Query datatypes now support issuing queries with field arguments
  • Field extended with arguments: BTreeMap<String, Argument>
  • As per QueryRequest for collections
• The reference connector now uses field arguments for testing purposes
  • Added limit to nested arrays
  • Added limit argument to the institutions.staff array field.
• The golden test suite makes assertions about the field arguments used by the reference agent
• NDC Spec now omits fields with required arguments

Follow Up Work

• Engine changes

Notes

• Schema and Query should probably be split into separate files to make it clear which phase the code is operating on but this should be done in a follow-up PR to make this change as clear as possible
• New schema and query fields have been made optional via serde parser directives in order to allow them to be omitted for backwards compatibility reasons.

[paf31]

Looks great so far, thanks. Couple of notes:

• I think we could come up with a better example than truncating the title: what about adding a where or limit argument to the nested arrays within institution? That would mirror a real use case.
• You'll also need to update the test cases for the reference agent to exercise these new code branches. I suspect tests are passing right now because the lack of an argument is ignored by the reference agent, but that should fail with a BadRequest error.
• ndc-test will have to be changed to omit any fields which take arguments, since we don't generate argument values in general.

[paf31]

We need to make the ndc-test change to omit fields with arguments, before this can merged, or else we'll start generating invalid requests.

The specification should also be updated please.

[sordina]

@paf31 there's a recursive case when if the only field in a query is omitted due to arguments, then the parent should also be omitted, etc.

[sordina]

@paf31 I think I've got everything updated:

• Fields are omitted from tests if they have any required arguments
• Arguments are now required to be explicitly null
• Debugging removed

I'm not sure how to use the ok_or pattern you recommended since we don't actually want to throw errors in the case of Nones in the reference agent unless I'm missing something. Let me know if you've got a sec to go over this.

[sordina]

@paf31

Yes, I thought we agreed we'd make all listed arguments required and revisit this everywhere in a follow up PR.

Yes I agree with this, but it feels like "limit" specifically is being required if any arguments are required.

In the current state of the test suite this is the same thing, but if any other arguments are added this no longer makes sense.

[sordina]

For example, it seems that if I added where too then and issued a query with a where field argument but not a limit, then it would error out because limit was missing.

[sordina]

@codedmart The issue is that we're not checking if all arguments are supplied, we're just checking if "limit" is supplied.

[paf31]

Let's not use the word optional because it'll be confusing (required/optional vs nullable/non-nullable). We could say all arguments are required, including nullable arguments.

[sordina]

- If a field has any arguments specified, then all arguments must be provided when that field is queried
- Literal argument `null` values may be used for nullable arguments

[sordina]

Is this ok?

[paf31]

I spotted a couple of bugs and I think I've fixed them, but I need to review. The new code also needs some tidying:

• ndc-test didn't exclude non-nullable columns in select_columns. That code now needs deduplicating.
• ndc-reference didn't parse arguments in the right place, leading us to not apply limit unless nested fields were requested.

Also, I noticed (and documented) that we can't support field arguments in column mappings for relationships, or to define relationship arguments, or in nested field path selectors. I think that'll be fine for now but I want to make sure that's ok @codedmart.

[sordina]

I think this incorrectly always requires a limit arg:

// ANCHOR_END: eval_comparison_target
// ANCHOR: eval_column
fn eval_column(
    variables: &BTreeMap<String, serde_json::Value>,
    row: &Row,
    column_name: &str,
    arguments: &BTreeMap<String, models::Argument>,
) -> Result<serde_json::Value> {
    let column = row.get(column_name).cloned().ok_or((
        StatusCode::BAD_REQUEST,
        Json(models::ErrorResponse {
            message: "invalid column name".into(),
            details: serde_json::Value::Null,
        }),
    ))?;

    if let Some(array) = column.as_array() {
        let limit_argument = arguments.get("limit").ok_or((
            StatusCode::BAD_REQUEST,
            Json(models::ErrorResponse {
                message: "Expected argument 'limit'".into(),
                details: serde_json::Value::Null,
            }),
        ))?;

failures:

---- tests::test_query stdout ----
thread 'tests::test_query' panicked at ndc-reference/bin/reference/main.rs:2416:22:
called `Result::unwrap()` on an `Err` value: (400, Json(ErrorResponse { message: "Expected argument 'limit'", details: Null }))

[sordina]

which makes having backwards compatibility impossible?