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 issue: Return 404 instead of 2xx codes for methods handling non-existent tasks#38 #69

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

vgopari
Copy link

@vgopari vgopari commented Feb 11, 2024

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Screenshot 2024-02-11 at 1 17 54 AM
Screenshot 2024-02-11 at 1 18 33 AM
Screenshot 2024-02-11 at 1 21 25 AM
Screenshot 2024-02-11 at 1 25 28 AM

Issue #

Alternatives considered

Describe alternative implementation you have considered

@JCHacking
Copy link
Contributor

Note that changing /poll and /poll/batch can break clients (since the expected codes were 200 with content if I get homework, and 204 without content if there was none).

If you also want those methods to give 404, this change should be labeled as BREAKING CHANGE.

@vgopari
Copy link
Author

vgopari commented Feb 16, 2024

@JCHacking, are you suggesting that we undo the code changes for /poll and /poll/batch to maintain the current behavior?

@JCHacking
Copy link
Contributor

@JCHacking, are you suggesting that we undo the code changes for /poll and /poll/batch to maintain the current behavior?

No, I think it is more correct that if it fails to get a task it returns 404.

But if you make this change you may have to change the client package as well and the SDKs in other languages.

@dcore94
Copy link

dcore94 commented Feb 17, 2024

I don't think that for poll it is more correct to return 404. This could be correct if task definition is not known because in that case it would actually reflect a possible error condition. 204 means operation has no errors but returns no content which is much more correct and it would not break anything.

@JCHacking
Copy link
Contributor

I don't think that for poll it is more correct to return 404. This could be correct if task definition is not known because in that case it would actually reflect a possible error condition. 204 means operation has no errors but returns no content which is much more correct and it would not break anything.

You right

@vgopari
Copy link
Author

vgopari commented Feb 23, 2024

I will revert the code to make the poll APIs return 204.

@vgopari
Copy link
Author

vgopari commented Mar 4, 2024

Changed the status of /poll to 204 No Content, incase of non-existent tasks.
image
image

@vgopari vgopari requested a review from v1r3n March 4, 2024 17:04
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.

5 participants