simplify-inflection: duplicate fields bug

[maximsmol]

Description

There seems to be a mistake in translating the simplify-inflection plugin from v4 to v5.

This causes naming conflicts in cases where there are two foreign-key references to a table:

create table users as (
	id bigint generated always as identity,
	primary key (id)
);

create table orders as (
	id bigint generated always as identity,
	primary key (id),

	purchased_by bigint not null
		references users(id),
	sold_by bigint not null
		references users(id)
);

This seems to inflect as

type Order {
	id: BigInt!

	user: User,
	user: User
}

with the simplify-inflection plugin.

Note, I didn't test the exact example, but it is a snippet from our actual schema with renamed tables/fields.

---

In the v4 version, the plugin compared detailedKeys to foreignTable.

In v5, the same code path goes through comparison between foreignPk.attributes and relation.remoteAttributes. Meanwhile, detailedKeys comes from relation.localAttributes. Also note that foreignPk comes from relation.remoteResource.uniques.

This means that we are essentially comparing relation.remoteResource.uniques to relation.remoteAttributes, while a direct translation from v4 would be a comparison between relation.localAttributes (≈ detailedKeys) and relation.remoteResource.uniques (≈ foreignTable).

Additionally, it is easy to see that this comparison does not at all depend on the local side of the specific field we are considering. Both fields come from relation.remote* fields, which explains why this would always produce the same name if the foreign-key target is the same.

---

The argument for singleRelationBackwards is basically the same. With the schema above, it causes the issue on the users table:

type User {
	id: BigInt!

	order: Order,
	order: Order
}

The same comparison can be made between the v5 and v4 code, except this time detailedKeys should be relation.remoteAttributes.

Performance impact

No impact.

Security impact

No impact.

Checklist

• My code matches the project's code style and yarn lint:fix passes.
• I've added tests for the new feature, and yarn test passes.
• I have detailed the new feature in the relevant documentation.
• I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• If this is a breaking change I've explained why.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 819fab3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[maximsmol]

Also there might be a similar issue with singleRelationBackwards, but I haven't figured it out yet
Edit: confirmed

[benjie]

Interesting. The plugin does assume a naming convention for FK relations that your schema doesn't comply with (see: https://github.com/graphile/crystal/tree/main/graphile-build/graphile-simplify-inflection#incompatible-database-schemas ), but if the fix is this simple it seems reasonable (I've not dug through your reasoning yet). It will be a semver major change, but that's fine for an inflection package - most changes will be semver major.

Please can you reproduce this issue with a test before the change, then we can see the diff afterwards?

[maximsmol]

I'll make a proper minimal reproduction sometime this week.

---

The aforementioned section of the docs is not very satisfying to me because we are simply trying to upgrade from v4 to v5, so changing up our schema isn't a viable solution. We have to hope that the updated plugins get us as close as possible to the same schema as before, and then pave over that with custom plugins. I think currently writing the compatibility plugin to bridge the new and the old behavior would be above my skill level with Postgraphile, though I personally can just run the patches from this PR, of course.

That being said, I'm only convinced to file this as an issue by my reasoning on comparing the two versions 1-to-1.

[maximsmol]

OK here is the repro: https://gist.github.com/maximsmol/b4b331c19d97d22d3bb1d512014ad7d3
I had to change it up a bit because I did not get the single-relation backwards case correct in my original draft (it is actually multi-relation backwards because it's not unique)

Included:

1. SQL schema
2. GraphQL config (only used for v5)
3. GraphQL schemas generated with
   • V4, no plugins
   • V4, @graphile-contrib/pg-simplify-inflector
   • V5, no plugins
   • V5, PgSimplifyInflectionPreset
   • V5, PgSimplifyInflectionPreset with this patch

Schemas were formatted with Prettier

Note that V5 with unmodified PgSimplifyInflectionPreset is the only scenario that creates duplicate names, debug error messages included in schema.sql

[benjie]

Sorry I wasn't clear, I meant reproduce it by adding a test to our test suite:

https://github.com/graphile/crystal/tree/main/graphile-build/graphile-simplify-inflection/tests

Is that something you're able to take a look at?

[maximsmol]

Yeah OK, got it. Should be pretty easy now that I have a reproducible example

[maximsmol]

I've added the test case, including the generated schemas and diff

I also included the diff without this patch as schema.graphql.bad.diff and the diff of the schemas before the patch and after patch as schema.graphql.patchimpact.diff

Edit: redid patchimpact to compare schemas rather than diffs since it was kind of unreadable before