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

Library cannot handle ID columns with names other than the default #41

Closed
csotiriou opened this issue Feb 10, 2019 · 5 comments
Closed

Comments

@csotiriou
Copy link

csotiriou commented Feb 10, 2019

It looks like although the code for composite keys is there in the src/index.js file, the initialisation of the adapter doesn't take into account the 'idColumn' property of the Objection model.

This has led me to failed queries all the time which seem inexplicable if one does not enable the 'debug' mode of the knex module to see the actual queries made.

Steps to reproduce the faulty behaviour:

  1. Go into any Objection model.
  2. Set the idColumn property of the model to something that does not exist
  3. Reboot your server. Make a GET /myModel/:modelID request. It shouldn't work, but it works. This means that the idColumn properly is not taken into account.

I have created a small patch to remedy this issue. @mcchrish Can you please check and merge?
#40

I believe that if this is OK, it justifies releasing 3.0.2 asap, as it is impossible to work with tables that has ID columns with name other than the name 'id;

@csotiriou csotiriou changed the title Doesn't take into account the 'idColumn' Library cannot handle ID columns with names other than the default Feb 11, 2019
@mcchrish
Copy link
Member

Thanks for opening a PR 👍

pinging @dekelev

@dekelev
Copy link
Member

dekelev commented Feb 12, 2019

@csotiriou Did you try to set the id property in the service options? https://github.com/mcchrish/feathers-objection#service-options

@csotiriou
Copy link
Author

I apologize. Seems I totally missed that part of the documentation. Sorry for that, and thank you for your answer, @dekelev

On a sidenote, however, is there any reason why this property is part of the service configuration and not the model? Seems more appropriate to be part of the model, since it is also part of how the Objection library works (and seems to me that it is also semantically more appropriate).

@dekelev
Copy link
Member

dekelev commented Feb 13, 2019

It's kind of a standard in feathers DB adapters, but I think it's a valid feature request to also use the model's idColumn in case the service.id is undefined. I'll check that over the weekend.

@dekelev
Copy link
Member

dekelev commented Feb 15, 2019

@dekelev dekelev closed this as completed Feb 15, 2019
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

No branches or pull requests

3 participants