-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
base: master
Are you sure you want to change the base?
Conversation
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 |
50fabfa
to
f3da297
Compare
@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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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:
...key, | |
key, |
const singleRelationFieldName = isUnique | ||
? inflection.singleRelationByKeysBackwards( | ||
keys, | ||
table, | ||
foreignTable, | ||
constraint | ||
) | ||
: uncoveredPrimaryKeys.length | ||
? inflection.rowByRelationBackwardsAndUniqueKeys( | ||
uncoveredPrimaryKeys, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncoveredPrimaryKeys, | |
uncoveredPrimaryKeys.map(details => details.key), |
primaryKeyConstraint && | ||
!omit(table, "single") && | ||
!omit(primaryKeyConstraint, "single") && | ||
!omit(constraint, "single")); |
There was a problem hiding this comment.
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 omit
s. 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")
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 schemac
.Not sure if separate (smaller) schema is needed to go over all edge cases as well, including renaming and custom inflection?