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(mongo): allow using different primary key types than ObjectId #568

Merged
merged 5 commits into from May 13, 2020

Conversation

ayazhussein
Copy link
Contributor

@ayazhussein ayazhussein commented May 12, 2020

Closes #349

@ayazhussein ayazhussein changed the title fix(core): added mongodb support for different primary keys types added mongodb support for different primary keys types May 12, 2020
package.json Outdated Show resolved Hide resolved
@ayazhussein
Copy link
Contributor Author

What do you propose?

@B4nan
Copy link
Member

B4nan commented May 12, 2020

Correct fix is to handle this based on the metadata. If the property type is not object id, we dont want to convert it.

It will be much more complex change, I don't know what and where, if I knew, it would be already implemented... But changing the platform methods is wrong I think, we should instead make sure they are not called for properties with non object id type. Also we need tests for the actual issue, where the PK is string, but it is 24 char hex string that can be converted to object id without problem (that is what #349 is about). We also need tests with numeric PK (as numbers can be also converted to object id without problems).

@B4nan
Copy link
Member

B4nan commented May 12, 2020

I already did some changes in this manner here: 86cd027

@ayazhussein
Copy link
Contributor Author

it doesn't cover all the cases, I did some changes, let me know if they are better

@B4nan
Copy link
Member

B4nan commented May 12, 2020

Good, much better! Will need more time to review it properly, but this looks like the correct way to me.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

ok this looks good, please fix those few code style changes i asked for and i think we are good to merge.

or do you think there are some uncovered edge cases? i would maybe suggest adding one more test that will be about numeric PK, but the code should handle this case properly, so it should be just about adding the test case to be sure.

packages/core/src/entity/EntityFactory.ts Outdated Show resolved Hide resolved
packages/core/src/entity/EntityFactory.ts Outdated Show resolved Hide resolved
packages/core/src/entity/EntityRepository.ts Outdated Show resolved Hide resolved
packages/mongodb/src/MongoDriver.ts Show resolved Hide resolved
@B4nan B4nan changed the title added mongodb support for different primary keys types feat(mongo): allow using different primary key types than ObjectId May 13, 2020
@B4nan B4nan mentioned this pull request May 13, 2020
46 tasks
@B4nan
Copy link
Member

B4nan commented May 13, 2020

Good, thanks again!

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

Successfully merging this pull request may close these issues.

None yet

2 participants