-
Notifications
You must be signed in to change notification settings - Fork 4
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
feature: Implement hasOne relation (issue #6) #28
feature: Implement hasOne relation (issue #6) #28
Conversation
Please review this PR and let me know if something needs to be changed @nicoabie |
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.
thanks again for contributing and sorry for the time it took me to review.
are you allowed to request my reviews?
src/assemble.test.ts
Outdated
test('assemble hasOne', () => { | ||
const objects = [{ 'p.personId': 1 }, { 'p.personId': 2 }]; | ||
|
||
const relations: Relations = { | ||
person: { | ||
contact: ['hasOne', 'contactDetails'], | ||
}, | ||
}; | ||
const aliases = { | ||
p: 'person', | ||
c: 'contact', | ||
}; | ||
const identifiers = ['p.personId', 'c.contactId']; | ||
|
||
assert.deepStrictEqual(assemble(relations, aliases, identifiers, objects), [ | ||
{ personId: 1 }, | ||
{ personId: 2 }, | ||
]); | ||
}); |
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.
is this a valid test case? I don't think the database is going to give that output
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.
test deleted
src/assemble.test.ts
Outdated
test('assemble hasOne relationship with complex data', () => { | ||
const objects = [ | ||
{ 'p.personId': 1, 'c.contactId': 101 }, | ||
{ 'p.personId': 2, 'c.contactId': null }, | ||
{ 'p.personId': 3, 'c.contactId': 101 }, | ||
{ 'p.personId': 4, 'c.contactId': 103 }, | ||
{ 'p.personId': 5 }, | ||
]; | ||
|
||
const relations: Relations = { | ||
person: { | ||
contact: ['hasOne', 'contactDetails'], | ||
}, | ||
}; | ||
const aliases = { | ||
p: 'person', | ||
c: 'contact', | ||
}; | ||
const identifiers = ['p.personId', 'c.contactId']; | ||
|
||
assert.deepStrictEqual(assemble(relations, aliases, identifiers, objects), [ | ||
{ personId: 1 }, | ||
{ personId: 2 }, | ||
{ personId: 3 }, | ||
{ personId: 4, contactDetails: { contactId: 103 } }, | ||
{ personId: 5 }, | ||
]); | ||
}); |
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 don't think this is correct.
- the database will return something for person 5, see my previous comment
- person 1 and 3 point to the same object and that is fine from a strict point of view
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.
fixed
src/assemble.ts
Outdated
@@ -77,6 +78,19 @@ export const assemble: AssembleFn = <T>( | |||
if (child[childIdentifier.split('.')[1]] !== null) { | |||
parent[parentChildRelation[1]].push(child); | |||
} | |||
} else if (parentChildRelation[0] === 'hasOne') { | |||
if (child[childIdentifier.split('.')[1]] !== null) { |
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.
maybe we should put childIdentifier.split('.')[1]
in a variable as it is being calculated 4 times
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.
fixed
ofcoure!! no worries, and no i don't think i have permission to request the review |
src/assemble.test.ts
Outdated
|
||
assert.deepStrictEqual(assemble(relations, aliases, identifiers, objects), [ | ||
{ personId: 1, contactDetails: { contactId: 101 } }, | ||
{ personId: 2 }, |
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.
in this case contactDetails should be null
{ personId: 2, contactDetails: null },
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.
fixed
src/assemble.test.ts
Outdated
test('assemble hasOne relationship with complex data', () => { | ||
const objects = [ | ||
{ 'p.personId': 1, 'c.contactId': 101 }, | ||
{ 'p.personId': 2, 'c.contactId': null }, | ||
{ 'p.personId': 3, 'c.contactId': 101 }, | ||
{ 'p.personId': 4, 'c.contactId': 103 }, | ||
]; | ||
|
||
const relations: Relations = { | ||
person: { | ||
contact: ['hasOne', 'contactDetails'], | ||
}, | ||
}; | ||
const aliases = { | ||
p: 'person', | ||
c: 'contact', | ||
}; | ||
const identifiers = ['p.personId', 'c.contactId']; | ||
|
||
assert.deepStrictEqual(assemble(relations, aliases, identifiers, objects), [ | ||
{ personId: 1 }, | ||
{ personId: 2 }, | ||
{ personId: 3 }, | ||
{ personId: 4, contactDetails: { contactId: 103 } }, | ||
]); | ||
}); |
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.
we can remove this one
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.
test removed
Implement hasOne relation for issue #6