graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.62k stars 571 forks source link

V5 @resultFieldName not working #1860

Closed frasern closed 1 month ago

frasern commented 1 year ago

Summary

The @resultFieldName Smart Tag no longer seems to work in V5. It is still in the docs and I couldn't find any mention of it being deprecated or replaced, so this appears to be a regression.

Steps to reproduce

  1. Define a function in Postgres and set the @resultFieldName smart tag:

    create function test_function(a int, b int)
    returns int
    as $$
       select a + b
    $$ language sql volatile;
    
    comment on function test_function(a int, b int) is E'@resultFieldName foo'; 
  2. Run the mutation in GraphQL:

    mutation MyMutation {
     testFunction(input: {a: 1, b: 2}) {
       foo
     }
    }

Expected results

✅ V4 gives the expected result, with the result field being correctly named foo:

{
  "data": {
    "testFunction": {
      "foo": 3
    }
  }
}

Actual results

❌ V5.0.0-beta.16 (running Postgraphile with only the amber preset npx postgraphile -P postgraphile/presets/amber -c postgres://...) gives an error for the same query:

{
  "errors": [
    {
      "message": "Cannot query field \"foo\" on type \"TestFunctionPayload\".",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "extensions": {
        "stack": [
          "GraphQLError: Cannot query field \"foo\" on type \"TestFunctionPayload\".",
          "    at Object.Field (/xxxxxxxxxx/node_modules/graphql/validation/rules/FieldsOnCorrectTypeRule.js:51:13)",
          "    at Object.enter (/xxxxxxxxxx/node_modules/graphql/language/visitor.js:301:32)",
          "    at Object.enter (/xxxxxxxxxx/node_modules/graphql/utilities/TypeInfo.js:391:27)",
          "    at visit (/xxxxxxxxxx/node_modules/graphql/language/visitor.js:197:21)",
          "    at validate (/xxxxxxxxxx/node_modules/graphql/validation/validate.js:91:24)",
          "    at parseAndValidate (/xxxxxxxxxx/node_modules/grafserv/dist/middleware/graphql.js:58:24)",
          "    at /xxxxxxxxxx/node_modules/grafserv/dist/middleware/graphql.js:359:38",
          "    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)",
          "    at async Server.<anonymous> (/xxxxxxxxxx/node_modules/grafserv/dist/servers/node/index.js:53:32)"
        ]
      }
    }
  ]
}

Additional context

It appears that default field name (result) always gets used instead of the one specified by the Smart Tag, so you can get the value by requesting the result field:

mutation MyMutation {
  testFunction(input: {a: 1, b: 2}) {
    result
  }
}
{
  "data": {
    "testFunction": {
      "result": 3
    }
  }
}

Possible Solution

Changing this function:

https://github.com/graphile/crystal/blob/775af9c6c6c99685890a4d06b8019f00317f3f77/graphile-build/graphile-build-pg/src/plugins/PgCustomTypeFieldPlugin.ts#L328-L330

to

functionMutationResultFieldName(options, details) {
    const { resource } = details;
    if (resource.extensions?.tags?.resultFieldName) {
        return resource.extensions.tags.resultFieldName;
    }
    return "result";
},

resolves the issue, but I don't know if that is the appropriate fix. It is based on some logic I found in PgV4InflectionPlugin.ts.

benjie commented 1 year ago

Please raise a PR to make the change you suggest :+1:

The V4 preset implements this smart tag, but the V5 preset does not; this was because I decided that consistently calling the results "result" would make more sense than having "boolean" and "float" all over the place. But if you want to explicitly change the name you should still be able to, and the smart tag is a strong signal of this, so I support the change you recommend :+1: