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 the ability to use PSCID in place of CandID in the candidates API. #8138

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Jul 14, 2022

This adds the ability to use PSCID instead of CandID in the URL for
the v0.0.4-dev version of the API. All endpoints under /candidates/{candID} should now accept /candidates/{PSCID} instead, as long as there's one single unique PSCID with that identifier.

This was requested by the HBCD project, but after talking to @ridz1208 is a common request for many projects, so I've added the functionality to v0.0.4-dev of the API.

@driusan
Copy link
Collaborator Author

driusan commented Jul 14, 2022

@xlecours Can you help me update the swagger schema and review? I looked into modules/api/static/schema-v0.0.4-dev.yml to try but it's not clear to me how to update it there..

@xlecours
Copy link
Contributor

xlecours commented Jul 14, 2022

That versioning is really annoying. I think it could go in v0.0.3 since it does not break anything.
Regardless of my opinion, yes, I'll look at the schema.

@xlecours
Copy link
Contributor

@driusan If you want to add in 0.0.4-dev, then you need to add the part that differ from 0.0.3 in modules/api/static/schema-v0.0.4-dev.yml (with the components definitions and all.) Otherwise, I would change candid for id at https://github.com/aces/Loris/blob/main/modules/api/static/schema.yml#L236 and every other instances in the path, as well as the parameter definition for each eandpoint.

@driusan
Copy link
Collaborator Author

driusan commented Jul 14, 2022

It would break scripts that assume it's the case on v0.0.3 on instances where it's not the case.

@driusan
Copy link
Collaborator Author

driusan commented Jul 14, 2022

But maybe it would be reasonable to release it as v0.0.4 and remove -dev?

@xlecours
Copy link
Contributor

But maybe it would be reasonable to release it as v0.0.4 and remove -dev?

And remove v0.0.3 ? I don't think we can afford to maintain multiple versions of the api.

This adds the ability to use PSCID instead of CandID in the URL for
the v0.0.4-dev version of the API. All endpoints under /candidates/{candID} should now accept /candidates/{PSCID} instead, as long as there's one single unique PSCID with that identifier.
@driusan
Copy link
Collaborator Author

driusan commented Aug 30, 2022

I incorporated the changes from the v0.0.4-dev into the 0.0.3 schema and then copied it back on top of the schema-v0.0.4-dev.yml file because 1. it more accurately represents what the endpoints are in v0.0.4-dev and 2. nearly every endpoint would have had to be copied anyways.

I had to do the candid->id changes manually because "candid" is a substring of "candidate" so it wasn't easy to do with a regex without breaking other things. It's possible I screwed something up.

I don't understand why we would remove 0.0.3 once 0.0.4 is released. It's 99% unchanged endpoints and usually only new ones are added. People are using the API and I don't see why we would need to remove it or have much maintenance to do before something breaks.

@ridz1208 I think @xlecours is away for a bit, can you review this now that the swagger schema is (hopefully) updated?

Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

Other than the versionning that is triggering me. I think this PR looks fine, the only usecase that this would not work is if the a study configures the PSCID to be formatted exactly like our CandID and it happens that one of the PSCIDS has a matching candID. I would say the odds of that are very low (I only know 1 study that uses that format) but a failsafe could be added to confirm that the PSCID configuration is NOT 6 digits

@xlecours
Copy link
Contributor

xlecours commented Sep 6, 2022

I think it should be mentioned that in the eventuality of a candid-pscid collision, the client should make sure that the returned candidate data matches the request.

@driusan
Copy link
Collaborator Author

driusan commented Sep 19, 2022

@xlecours @ridz1208 added a note.. is this good to be merged now?

@ridz1208
Copy link
Collaborator

I already approved. all good here

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

I don't approve the feature not the schema file changes but the code is ok. And, on a side note, this introduce the first sql statement in the api module.

@driusan
Copy link
Collaborator Author

driusan commented Sep 19, 2022

Good enough for me!

@driusan driusan merged commit 8f96a2a into aces:24.1-release Sep 19, 2022
@driusan driusan added this to the 24.1.0 milestone Sep 20, 2022
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.

3 participants