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

Allow single-part response for retrieve instance #43

Merged
merged 4 commits into from
Dec 14, 2020

Conversation

hackermd
Copy link
Collaborator

@hackermd hackermd commented Dec 3, 2020

Some origin servers respond to retrieve instance requests with a message containing only a single part. While this is not compliant with the standard, the client should be able to handle this behavior. It will log an error message to make users aware of the standard compliance issue.

@hackermd
Copy link
Collaborator Author

hackermd commented Dec 3, 2020

@pieper @ntenenz what are your thoughts regarding the Accept header? One the one hand, I think we should now include application/dicom in the list of acceptable media types to prevent a server from responding with a 406 error code although we would actually accept the content type. On the other hand, I am concerned that this may break things with some servers. We had issues with origin servers in the past that did not correctly implement HTTP.

@pieper
Copy link
Member

pieper commented Dec 4, 2020

Here I'd side on sticking to the standard and not offer to accept a non-standard type. If the user defined Accept in the headers param would it override the client code or vise versa?

@hackermd
Copy link
Collaborator Author

hackermd commented Dec 4, 2020

Here I'd side on sticking to the standard and not offer to accept a non-standard type.

That was my initial thought, too.

If the user defined Accept in the headers param would it override the client code or vise versa?

Good question. I haven't thought about the possibility of someone setting the Accept header for the entire session. It looks like headers passed via the request method will override the headers that have been set on the session object (see requests.sessions.merge_settings()).

@hackermd
Copy link
Collaborator Author

@pieper @ntenenz can we complete this PR?

@ntenenz
Copy link
Contributor

ntenenz commented Dec 12, 2020

LGTM

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

👍

@hackermd
Copy link
Collaborator Author

@pieper @ntenenz Great! Thanks for your feedback. Could one of you kindly approve the PR? Then I will merge the changes, draft a new release and publish a new version on pypi.

Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Odd, I thought I approved yesterday but I must not have clicked the right button...

@pieper
Copy link
Member

pieper commented Dec 13, 2020

It happened again - I clicked "Add your review" and selected "Approve" but that did not clear the "Review required" flag. I guess I don't have write access here.

@ntenenz
Copy link
Contributor

ntenenz commented Dec 13, 2020

Steve and I are both showing as have approved the PR. I agree, it looks like neither of us have write access.

@hackermd hackermd merged commit 0183680 into master Dec 14, 2020
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