Throw error when mapping to source's own property while primary key belongs to another source #2574
Conversation
core/src/modules/mappingHelper.ts
Outdated
| if (otherSourcePrimaryKey && property.sourceId === instance.id) { | ||
| throw new Error( | ||
| `'${instance.name}' cannot map '${remoteKey}' to own Property '${key}'. '${source.name}' must map to a Property from primary Source '${otherSourcePrimaryKey.source.name}'.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
I'm still thinking about the wording. I think in the case this happen the primary source is ambiguous. For example if the Users and Purchases source point to their own properties, it's not clear which is the primary source. I'm open for feedback.
Maybe it should just say while {sourceB} is mapping to its own property {propertyB} ?
There was a problem hiding this comment.
What about something like
`${otherSourcePrimaryKey.soruce.name} is the primary source for the ${instance.model.name} Model - ${instance.name} requires a mapping back to ${otherSourcePrimaryKey.soruce.name}`users-table is the primary source for the profiles - purchases-table requires a mapping back to users-table`
| @@ -634,9 +634,18 @@ describe("models/property", () => { | |||
| }); | |||
|
|
|||
| describe("with plugin", () => { | |||
| // let model: GrouparooModel; | |||
| test("will throw error when mapping to own property and primary key is owned by other source", async () => { | ||
| await expect(source.setMapping({ email: "myEmail" })).rejects.toThrow( | ||
| /cannot map 'email' to own Property 'myEmail'/ | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This is the new test - the other changes are cleanup / clarifications based on this new behavior. Initially I would have guessed this change would have required a more complex series of tests... but... it doesn't! We've got other tests confirming that the first property (bootstrap) still works like expect
| ? await Property.findOne({ | ||
| include: { | ||
| model: Source, | ||
| required: true, | ||
| where: { | ||
| modelId: instance.modelId, | ||
| }, | ||
| }, | ||
| where: { | ||
| isPrimaryKey: true, | ||
| sourceId: { | ||
| [Op.ne]: instance.id, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
👍 great single query to find this!
core/src/modules/mappingHelper.ts
Outdated
| if (otherSourcePrimaryKey && property.sourceId === instance.id) { | ||
| throw new Error( | ||
| `'${instance.name}' cannot map '${remoteKey}' to own Property '${key}'. '${source.name}' must map to a Property from primary Source '${otherSourcePrimaryKey.source.name}'.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
What about something like
`${otherSourcePrimaryKey.soruce.name} is the primary source for the ${instance.model.name} Model - ${instance.name} requires a mapping back to ${otherSourcePrimaryKey.soruce.name}`users-table is the primary source for the profiles - purchases-table requires a mapping back to users-table`
…elongs to another source * Update tests * Add new test to verify conditon * Improve error logging in Property when checking for uniue property
edmundito
force-pushed
the
180403934-primarykey-cfg-validation
branch
from
8e8aa34
to
e22196f
Compare
This change ensures that it's not possible to set a mapping to a property in the source when another source owns the primary key by throwing an error.
When running
grouparoo configwhere two different sources (PG usersandPG purchases) in the same model point to their own properties in the mapping, we see an error such as:Related: #2572
Change description
Description here
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
In the case that a customer had already setup a configuration where two sources were mapping to their own keys, they would need to update the mapping in the config files.
Code review