server: return single object from non-SETOF SQL functions

[kraftwerk28]

Description

When applying an SQL function for table computed field, the function might either return:

1. a scalar (bool, int, decimal etc)
2. table_name, where table_name is tracked by Hasura. The return value is a single object
3. SETOF table_name, where table_name is tracked by Hasura. The return value is an array of objects

The issue is that for 2nd case, Hasura generates SETOF-like schema, i.e. the return value is an array with a single object in it. This is somewhat inconvenient, as the code which accesses the computed field value, must always index into the array.

A simplified example:

create table "company" (
  company_id bigserial primary key,
  company_name text not null unique
);
create table "user" (
  user_id bigserial primary key,
  username text not null unique,
  company_id bigint not null references "company" (company_id)
);

-- This can be achieved using object relationships, but for the sake of example...
create function user_company("user")
  returns "company"
  language sql
  immutable
as $$
  select * from "company" c
  where c.company_id = $1.company_id
  limit 1;
$$;

Track user_company function as computed field for "user" table.

Then populate database using following query:

mutation A {
  insert_company_one(object: {company_id: 1, company_name: "Nokia"}) {
    company_id
  }
  insert_user_one(object: {user_id: 1, username: "John", company_id: 1}) {
    user_id
  }
}

Now, query

query B {
  user {
    id: user_id
    username
    company_com {
      id: company_id
      company_name
    }
  }
}

returns

{
  "data": {
    "user": [
      {
        "id": 1,
        "username": "John",
        "company_com": [
          {
            "id": 1,
            "company_name": "Nokia"
          }
        ]
      }
    ]
  }
}

instead of desired

{
  "data": {
    "user": [
      {
        "id": 1,
        "username": "John",
        "company_com": {
          "id": 1,
          "company_name": "Nokia"
        }
      }
    ]
  }
}

Changelog

Component : server

Type: bugfix / feature / enhancement (not sure)

Product: community-edition

Short Changelog

server: return single object from non-SETOF SQL functions

Long Changelog

Related Issues

#4678, #6562

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

• [x] No
• [ ] Yes
  • [ ] Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

• [x] No
• [ ] Yes
  • Does run_sql auto manages the new metadata through schema diffing?
    • [ ] Yes
    • [ ] Not required
  • Does run_sql auto manages the definitions of metadata on renaming?
    • [ ] Yes
    • [ ] Not required
  • Does export_metadata/replace_metadata supports the new metadata added?
    • [ ] Yes
    • [ ] Not required

GraphQL

• [x] No new GraphQL schema is generated
• [ ] New GraphQL schema is being generated:
  • [ ] New types and typenames are correlated

Breaking changes

• [ ] No Breaking changes

• [x] There are breaking changes:

  1. Metadata API

     Existing query types:

     • [ ] Modify args payload which is not backward compatible
     • [ ] Behavioural change of the API
     • [ ] Change in response JSON schema
     • [ ] Change in error code

  2. GraphQL API

     Schema Generation:

     • [ ] Change in any NamedType
     • [ ] Change in table field names
     • [x] Change in table field shape

     Schema Resolve:-

     • [ ] Change in treatment of null value for any input fields

  3. Logging

     • [ ] Log JSON schema has changed
     • [ ] Log type names have changed

[hasura-bot]

Beep boop! :robot:

Hey @kraftwerk28, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible.

Stay awesome! :sunglasses:

[nosovk]

@rikinsk could you please approve CI builds for PR? It will also allow us to build that one https://github.com/hasura/graphql-engine/pull/8728

[plcplc]

Hi @kraftwerk28.

Thanks for making this contribution, and thanks for a very articulate PR description.

This looks like a change we would like to include. I'm going to be reviewing the PR and likely also push some additional commits that add tests of the feature.

[plcplc]

Okay, so currently there is one issue with this solution in that the schema for a company computed field still indicates that the field has a list-type:

type User {
  company(
    distinct_on: [company_select_column!]
    limit: Int
    offset: Int
    order_by: [company_order_by!]
    where: company_bool_exp): [company!]
  company_id: bigint!
  user_id: bigint!
  username: String!
}

(console screengrab)

Ideally, its type should be:

type User {
  company: company # note nullability and the absence of input fields.
  company_id: bigint!
  user_id: bigint!
  username: String!
}

which would reflect that its only a single, nullable field.

[kraftwerk28]

@plcplc, that is something I missed, thank you. Is there any way to build&publish this branch on CI to test out my changes after I fix the issue? My main machine with 16GB RAM + 4GB swap is unable to build the graphql-engine, ghc is simply OOM-killed.

[plcplc]

Is there any way to build&publish this branch on CI to test out my changes after I fix the issue? My main machine with 16GB RAM + 4GB swap is unable to build the graphql-engine, ghc is simply OOM-killed.

I'm going to have a look and see if we support CI for community-contributed PRs.

Unless you have already, I can recommend turning off compiler optimizations for the graphql-engine library by adding flags: -optimize-hasura at the top-level of cabal.project.local.

[nosovk]

Thanks, flags: -optimize-hasura improved memory consumption and was able to run it locally

[Stefmore02]

Hey! @kraftwerk28

This is a friendly reminder to address the comments/changes made to proceed with this PR. If any questions, don't hesitate to reach out! 🤓

[nosovk]

@Stefmore02 could you allow running CI on a branch?

[kraftwerk28]

It now works as expected, non-SETOF computed fields don't have where / limit / offset / order by / distinct clauses and are single value, nullable.

[plcplc]

@kraftwerk28 I think this is shaping up pretty good.

It would help it along if you could add a test of this in the server/lib/api-tests test suite. This should be relatively easy if you simply copy one of the existing tests of computed fields and adapt it.

[kraftwerk28]

Note, that this PR introduces the following changes to the GraphQL schema of computed fields:

1. SETOF <table>: The return type changed from [some_type!] to [some_type!]!. That is, if a SETOF function returns NULL, it means an empty, non-nullable array (so, no NULL / [] ambiguity)
2. <table>: The return type changed from [some_type!] to some_type

[kraftwerk28]

@plcplc, I've added tests for Postgres and BigQuery. Because BigQuery doesn't support SETOF modifier, the computed fields for this backend type always act as if the function was SETOF ([some_type!]! in GQL).

[plcplc]

@kraftwerk28 : Thanks for this.

As far as developing the actual feature I think this PR is in good shape. However, since it does make potentially breaking changes to the schema which are not opt-in (because an instance having a non-setof computed field will see the type of that change from [a] to a! when it is upgraded), we need to be mindful of how we actually introduce such a change into the graphql-engine.

Currently our process for handling that is not particularly streamlined, and we're deliberating internally how to accommodate changes such as this one as well of several others.

I don't know exactly how we're going to proceed. It may be that we'll add a means to make the change opt-in, but since we're yet undecided I won't ask you to come up with that :-)

[nosovk]

It would be nice to have it in graphql-engine. We have some pain with that issue, the solution is not something out of nowhere, its an issue from real project.

[plcplc]

It would be nice to have it in graphql-engine. We have some pain with that issue, the solution is not something out of nowhere, its an issue from real project.

Yeah, I can appreciate that. There's no question that this is, by itself, purely a good change. I'll try and get back onto this as soon as we have made up our minds about how to proceed!

[kraftwerk28]

It may be that we'll add a means to make the change opt-in

Making a command-line/env-variable flag which would change the behavior is a no-issue for me, although it doesn't look like a good solution. Instead, why wouldn't you merge PR's like this one into the next major release? Following semver spec, a new major means possible breaking changes anyway.

[TRONJon]

I myself have ran into this issue with several new projects. If there's any chance of getting this PR through (without breaking backwards compatibility with old schemas) then my dev team will be incredibly grateful 🙇

[dariocravero]

Hey all, it's been a while since the last conversations here. What's the likelihood of this getting merged at the end? Would love to see this in. Thanks!