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

🐛 Fix #295 , 500 server errors on POST to sequencing-centers. #296

Closed
wants to merge 2 commits into from
Closed

🐛 Fix #295 , 500 server errors on POST to sequencing-centers. #296

wants to merge 2 commits into from

Conversation

fiendish
Copy link
Contributor

@fiendish fiendish commented May 3, 2018

Fixes #295 , 500 server errors on POST to sequencing-centers.
See marshmallow-code/flask-marshmallow#44

Fixes 500 server errors on POST to sequencing-centers.
See marshmallow-code/flask-marshmallow#44
@fiendish fiendish added the bug Something isn't working label May 3, 2018
@fiendish fiendish requested a review from parimalak May 3, 2018 15:29
@fiendish fiendish changed the title fix for #295 🐛 Fix #295 , 500 server errors on POST to sequencing-centers. May 3, 2018
@fiendish fiendish changed the title 🐛 Fix #295 , 500 server errors on POST to sequencing-centers. 🐛 Fix #295 , 500 server errors on POST to sequencing-centers. May 3, 2018
@parimalak parimalak requested review from dankolbman and znatty22 May 3, 2018 20:08
@dankolbman
Copy link
Contributor

What was the error being thrown here? Can it be reproduced + tested?

@fiendish
Copy link
Contributor Author

fiendish commented May 3, 2018

Both sides of the error are described in #295
But to reproduce you can throw curl -XPOST -H "Content-Type: application/json" http://kf-api-dataservice-qa.kids-first.io/sequencing-centers -d '{"kf_id": "SC_K52V7463", "name": "Washington University"}' into your terminal and see the error come back.

@dankolbman
Copy link
Contributor

Hmm ok, I'm guessing this is due to the fact that it needs to make a query in the serializer to ensure that the name is unique

Copy link
Contributor

@dankolbman dankolbman left a comment

Choose a reason for hiding this comment

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

💯
Remember to rebase on master manually first before merging.

@znatty22
Copy link
Member

znatty22 commented May 3, 2018

I was able to reproduce it locally. So we may want to hold off on merging this PR and make another PR to address the larger problem. I've confirmed that this happens any time you try to POST any entity with a given kf_id. Also, not sure about this but I think we may need to add in some validation on kf_ids (ensuring that the correct prefixes exist depending on the entity). We may as well do that all in one PR

@dankolbman
Copy link
Contributor

I see. Looks like maybe we wanted to include a kf_id for dump only in the base schema: https://github.com/kids-first/kf-api-dataservice/blob/master/dataservice/api/common/schemas.py#L23

We may want to change this, though, for #293 and #294

@dankolbman
Copy link
Contributor

#308 is a general fix for this problem.

@dankolbman
Copy link
Contributor

dankolbman commented May 7, 2018

Fixed by #308

@dankolbman dankolbman closed this May 7, 2018
@dankolbman dankolbman removed the ready-for-review This PR needs a review label May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants