Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pg) single backward relations with arguments if primary key covered partially #567

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ab-pm
Copy link
Contributor

@ab-pm ab-pm commented Nov 19, 2019

Integrating the plugin I had posted on Discord into core.

Basic tests (creates the fields it should, queries work as expected, can be omitted using new @omit single #460) included based on the existing test schema c.
Not sure if separate (smaller) schema is needed to go over all edge cases as well, including renaming and custom inflection?

@ab-pm
Copy link
Contributor Author

ab-pm commented Nov 19, 2019

Does this need to go behind a feature flag? I figured this might break existing schemas that a) have such foreign references on parts of their primary keys and b) have a @foreignFieldName that doesn't properly singularise, leading to a name collision between the "many" backwards relation and the new single-but-parameterised relation.

@benjie
Copy link
Member

benjie commented Jan 7, 2020

@ab-pm Thanks for the PR. I'm going to leave this hanging for a while because I want to figure out what I'm doing with the existing core plugins in v5 before extending the codebase further.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this PR and it's a cool feature, but I think we're going to be changing so much of the fundamentals in v5 that I can't currently merge it. I'm going to leave it open and merge parts of it into my v5 works when I'm ready. There's not much point changing it more right now because some of the interfaces it currently uses are going to be removed in v5.

? primaryKeys.filter(attr => !keys.includes(attr))
: [];
const parameterKeys = uncoveredPrimaryKeys.map(key => ({
...key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's less risk of conflicts here if we don't splat this out:

Suggested change
...key,
key,

const singleRelationFieldName = isUnique
? inflection.singleRelationByKeysBackwards(
keys,
table,
foreignTable,
constraint
)
: uncoveredPrimaryKeys.length
? inflection.rowByRelationBackwardsAndUniqueKeys(
uncoveredPrimaryKeys,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uncoveredPrimaryKeys,
uncoveredPrimaryKeys.map(details => details.key),

primaryKeyConstraint &&
!omit(table, "single") &&
!omit(primaryKeyConstraint, "single") &&
!omit(constraint, "single"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of potential omits. I'd like to really tighten up how omit works in V5, I think in this case it should be something like !omit(constraint, "backward")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants