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

Add two routeOptions properties modelCustomIdType and modelCustomChildI… #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucarp
Copy link
Contributor

@lucarp lucarp commented Jan 11, 2018

Add two routeOptions properties modelCustomIdType and modelCustomChildIdType to allow custom id types in generated routes to pass Joi validations.

With this change, one can use different types of _id in different routes.

…dType to allow custom id types in generated routes.
@JKHeadley
Copy link
Owner

I really like this idea, and I think it could be beneficial to many users. There are a few things I'm concerned about or unclear about that maybe you could help me think through.

  1. It seems that the actual routeOptions properties that are checked are cusomtIdType and customChildIdType rather than modelCustomIdType and modelCustomChildIdType. Is that what you intended?

  2. I noticed there is no option for specifying the _id type with the create endpoint, which means it will still default to an objectId. Could you help me understand the type of workflow you are expecting here?

  3. I am a bit cautious in adding this feature simply because the _id property is very tightly integrated with many aspects of the framework. For instance, any associations would also need to make sure they reference the correct _id type. You can check out joi-mongoose-helper.js for examples. There might be other situations that would be affected that I can't currently think of.

  4. Given that my previous points are accounted for, I would definitely want this feature to be well tested. Unfortunately testing could prove to be a major undertaking considering the amount of integration affected by this feature. The easiest solution might be to duplicate the current end-to-end tests and provide a varied mix of custom _id types to make sure the core features function correctly.

@lucarp
Copy link
Contributor Author

lucarp commented Jan 12, 2018 via email

@JKHeadley
Copy link
Owner

Thanks for the explanation. I believe there might be a better solution that involves auto-generating the Joi object based on the actual _id type. I will look into this today and let you know so we can get this feature out ASAP.

@JKHeadley
Copy link
Owner

@lucarp Just to update, I created a new branch looking into the approach I mentioned earlier. You can take a look at my last commit to get an idea as to what I am trying to accomplish.

I believe this update accomplishes the Joi validation side of custom _ids, however after looking at other parts of the code there are still many areas that will need updating. For example, creating model associations still assumes documents are using ObjecIds for their _ids. I would be happy to work with you on getting this feature implemented, however it looks like its going to take some work.

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.

2 participants