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

Feature: add support for service.id as array of columns #194

Closed
dekelev opened this issue Apr 9, 2020 · 6 comments
Closed

Feature: add support for service.id as array of columns #194

dekelev opened this issue Apr 9, 2020 · 6 comments
Milestone

Comments

@dekelev
Copy link
Member

dekelev commented Apr 9, 2020

When integrating this lib with feathers-objection & feathers-cassandra services, service.id property can be an array of column names that represent a composite PK.

In addition, there's the service.idSeperator property that default to , so a composite PK can be set as ID with REST, e.g. /users/firstPathParam,secondPathParam.

I didn't find any workaround for this issue in the lib options.
The only thing that worked was changing the file feathers-swagger/openapi.js:line 190 to:

swaggerPath += `/{${Array.isArray(methodIdName) ? methodIdName.join(`}${service.idSeparator}{`) : methodIdName}}`;

Is there anything I'm missing here? should we integrate this logic into the lib or expose a method in the options to control it?

@dekelev dekelev changed the title Missing support for service.id as array of columns Add support for service.id as array of columns Apr 9, 2020
@dekelev dekelev changed the title Add support for service.id as array of columns Feature: Add support for service.id as array of columns Apr 9, 2020
@dekelev dekelev changed the title Feature: Add support for service.id as array of columns Feature: add support for service.id as array of columns Apr 9, 2020
@Mairu
Copy link
Collaborator

Mairu commented Apr 9, 2020

Hi, I guess it should be part of the lib itself, but there are more things that would need to be changed. service.id is used in other places too.

@dekelev
Copy link
Member Author

dekelev commented Apr 9, 2020

Cool, I'll PR this later with tests for your review. thanks!

@Mairu
Copy link
Collaborator

Mairu commented Apr 9, 2020

PRs are welcome. As a hint, if you need it fast, you should target v1 branch, I guess master (and v2) will still take a while. Of course this would only be valid if there are no BC breaks.

@dekelev
Copy link
Member Author

dekelev commented Apr 9, 2020

I've opened a PR on v2 (master branch). I'll check v1 soon.
There are no breaking-changes.

@dekelev
Copy link
Member Author

dekelev commented Apr 9, 2020

Added another PR on v1 branch.

@Mairu
Copy link
Collaborator

Mairu commented Apr 13, 2020

Merged it into v1 branch.

@Mairu Mairu closed this as completed Apr 13, 2020
@Mairu Mairu added this to the 1.2.0 milestone Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants