Upgrading from v2.8.4 to v2.20.0 introduces unexpected graphql-default naming convention on filter fields

zoidbergSF commented 1 year ago

Version Information

Server Version: 2.20.0

Environment

OSS

What is the current behaviour?

When updating our server from v2.8.4 to v2.20.0 we get unexpected field name changes on the generated schema causing breaking changes to our API. We have the HASURA_GRAPHQL_EXPERIMENTAL_FEATURES: naming_convention environment variable set, and we do not have the HASURA_GRAPHQL_DEFAULT_NAMING_CONVENTION: graphql-default variable set.

We have 2 postgres databases connected with the following configuration in the databases.yaml file:

- name: primary
  kind: postgres
  #...
- name: device
  kind: postgres
  customization:
    naming_convention: graphql-default
  #...

After updating to v2.20.0 there are field name changes for filters in queries on tables of the primary database even though it does not have naming_convention: graphql-default set in its configuration, for example:

users(where: {deletedAt: {_is_null: false}}) {
    #...
  }

becomes

users(where: {deletedAt: {_isNull: false}}) {
    #...
  }

What is the expected behaviour?

Upgrading from server version v2.8.4 to v2.20.0 should not introduce filter field name changes to the schema generated from the primary database which does not have naming_convention: graphql-default configured.

Keywords

naming_convention
graphql-default

tirumaraiselvan commented 1 year ago

@zoidbergSF Do you have naming_convention in any of your source metadata by any chance?

zoidbergSF commented 1 year ago

Yes, we recently added a second postgres database to our server, which has the naming_convention: graphql-default set in its customization in the databases.yaml file. The primary database was configured some time ago already before naming_convention options became available.

tirumaraiselvan commented 1 year ago

This metadata setting will set the naming convention as it overrides the environment variable configuration. This is expected behavior. Did you not intend to add the naming_convention to your source?

On Fri, Mar 24, 2023, 3:31 PM zoidbergSF @.***> wrote:

Yes, we recently added a second postgres database to our server, which has the naming_convention: graphql-default set in its customization in the databases.yaml file. The primary database was configured some time ago already before name_convention options became available.

— Reply to this email directly, view it on GitHub https://github.com/hasura/graphql-engine/issues/9523#issuecomment-1482541206, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCDEEVV36Y43WXBTW4WL53W5VWGXANCNFSM6AAAAAAWGJRYVM . You are receiving this because you commented.Message ID: @.***>

zoidbergSF commented 1 year ago

@tirumaraiselvan thanks for the reply,

yes we did intend to add the naming_convention to the second postgres device database when we added it as a source, and the names on these tables were generated as expected, And since we configured the graphql-default naming_convention only on that database in the databases.yaml file, we did not see any changes to the schema generated on the primary database, again as expected.

This was all done while running v2.8.4 without encountering issues. However this week while upgrading to v2.20.0 and making no changes, we see the generated field names change for filtering options on tables in the primary database, as seen in the users query above.

This is unexpected because:

we do not have naming_convention: graphql-default specified in the customization options of the primary database, therefore didn't expect the query field names to have changed to the graphql-default convention.
this was working as expected while running version v2.8.4 and field names only changed when updating to v2.20.0, even though we did not change anything else other than updating the hasura version.

paritosh-08 commented 1 year ago

@zoidbergSF, thanks for raising the issue. I could not reproduce this issue. Can you please help me out? I did the following:

Start a graphql-engine v2.8.4 (in docker) with HASURA_GRAPHQL_EXPERIMENTAL_FEATURES: naming_convention.
Add two sources (default and device)

Click to see the metadata
```json { "resource_version": 4, "metadata": { "version": 3, "sources": [ { "name": "default", "kind": "postgres", "tables": [ { "table": { "schema": "public", "name": "some_test_table" } } ], "configuration": { "connection_info": { "use_prepared_statements": true, "database_url": { "from_env": "HASURA_GRAPHQL_DATABASE_URL" }, "isolation_level": "read-committed", "pool_settings": { "connection_lifetime": 600, "retries": 1, "idle_timeout": 180, "max_connections": 50 } } }, "customization": { "naming_convention": "graphql-default" } }, { "name": "device", "kind": "postgres", "tables": [ { "table": { "schema": "public", "name": "users" } } ], "configuration": { "connection_info": { "use_prepared_statements": false, "database_url": { "from_env": "HASURA_GRAPHQL_DATABASE_URL_2" }, "isolation_level": "read-committed" } } } ] } } ```
Observe that the field exposed from the device source is following the hasura-default naming convention.

Click to see an example of valid graphql query
```gql query MyQuery { users(where: {deleted_at: {_is_null: false}}) { id deleted_at } } ```
Shut down the server, change the graphql-engine image to hasura/graphql-engine:v2.20.0, and start again.
It succeeded when I tried running the same query (see the query above).

After updating to v2.20.0 there are field name changes for filters in queries on tables of the primary database even though > it does not have naming_convention: graphql-default set in its configuration, for example:
users(where: {deletedAt: {_is_null: false}}) {
    #...
  }
becomes
users(where: {deletedAt: {_isNull: false}}) {
    #...
  }

As far as I can tell, the users field is exposed from a source that follows the graphql-default naming convention in both cases. In v2.20.0, we fixed a bug where a few operators were not changing based on the naming convention and _is_null was one of them. I guess this might be the reason why the _is_null operator became _isNull in v2.20.0. The commit for the mentioned bugfix: https://github.com/hasura/graphql-engine/commit/96549b272b07d5fdb82207829104ae626f4c6cb3

hasura / graphql-engine