feat: Add ability to specify the records id when calling model.create() #679
Conversation
Codecov Report
@@ Coverage Diff @@
## master #679 +/- ##
=========================================
+ Coverage 92.28% 92.3% +0.01%
=========================================
Files 180 180
Lines 1996 2000 +4
=========================================
+ Hits 1842 1846 +4
Misses 154 154
Continue to review full report at Codecov.
|
jamemackson
force-pushed
the
master
branch
2 times, most recently
from
0b5c016 to
fa50ba4
Compare
|
@zacharygolba I updated this to use a property on the model that enables the alternate behavior to allow, on a model specific basis, inserting with a specified id. I'm sure there are still a lot of ways this could be better... I'm all ears for any suggestions you have. |
src/packages/database/model/index.js
Outdated
| * @default 'id' | ||
| * @public | ||
| */ | ||
| allowPrimaryKeyInsert: boolean = false; |
There was a problem hiding this comment.
I think that this flag is unnecessary. The ORM should be smart enough to know if a record has an id in it's change set and it has not been persisted to the database yet.
| @@ -23,6 +23,24 @@ export function create(record: Model, trx: Object): Array<Object> { | |||
| updatedAt: timestamp | |||
| }); | |||
|
|
|||
| if (record.allowPrimaryKeyInsert) { | |||
| const columns = getColumns(record); | |||
| // check that we have a valid id here? | |||
There was a problem hiding this comment.
I don't think that we have to check if the id is valid. If this fails due to an error that is thrown by the database driver that is ok. No need for us to add an additional error message and spend more time in node than what is necessary.
jamemackson
changed the title
| isPublic: true | ||
| }); | ||
|
|
||
| expect(result).to.be.an.instanceof(Subject); |
There was a problem hiding this comment.
I feel like this assertion and expect(result).to.have.property('id', id); from L403 are all that is required to ensure that you can manually insert ids. Maybe w could remove all of the other expect calls and the unnecessary constants for this test case.
|
@zacharygolba thanks for the feedback, all great suggestions. I hope the modifications to the test was inline with what you were thinking. Let me know... :) |
|
Looks good @jamemackson. Awesome work! |
|
thanks @zacharygolba!! |
@zacharygolba here's the first pass, just pushing to start the discussion on the implementation of this.
refs: #678