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.

Sign up for GitHub

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

Jump to bottom

dcs: Remap error codes from TTJS #7143

Merged
merged 6 commits into from
Jun 18, 2024
Merged

dcs: Remap error codes from TTJS #7143

merged 6 commits into from
Jun 18, 2024

Conversation

halimi
Copy link
Contributor

@halimi halimi commented Jun 14, 2024

Summary

Closes #7045

HTTP error codes coming from the JS passed through to the client. These error codes are not relevant to the clients and sometimes they are confusing e.g. wrong owner token results 403 forbidden but it should be just 400 bad request. To not confuse the client we remapped some of the error codes.

Changes

This change remaps the error code from TTJS.

  • HTTP error code 400 bad request and 401 unauthorized to 500 internal server error
  • HTTP error code 403 forbidden to 400 bad request

Testing

Steps

Try to register a device with a wrong authentication code and you should get error 400 bad request instead of 403 forbidden.

Results

Registering a device with wrong authentication code results error code 400.

Regressions

Some of the error codes have been changed.

Notes for Reviewers

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@halimi halimi requested a review from a team as a code owner June 14, 2024 09:31
johanstokking
johanstokking reviewed Jun 14, 2024
View reviewed changes
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

@halimi this does not implement the proposal of #7045, e.g. it cannot be simply be mapping error codes as some status codes mean two different things where we'd need to check the error message.

@halimi
Copy link
Contributor Author

halimi commented Jun 14, 2024

@johanstokking you are right, I will change it to check the error message. Is the error message claim failed with given owner token in the proposal correct? I cannot find it in the code.

@halimi halimi marked this pull request as draft June 14, 2024 12:25
@johanstokking
Copy link
Member

johanstokking commented Jun 14, 2024 •
edited
Loading

@johanstokking you are right, I will change it to check the error message. Is the error message claim failed with given owner token in the proposal correct? I cannot find it in the code.

It comes from here: https://github.com/TheThingsIndustries/lorawan-join-server/blob/master/src/core/controller.ts#L486-L488.

The response will be a JSON message {"message":"claim failed with given owner token"} with status code 403. That needs to be mapped to 400 bad request. The others should become 500. So check if the Content-Type is application/json, then unmarshal JSON into a struct with Message string json:"message"` and then check the string. Ideally add a comment on the aforementioned line in TTJS to not change the error message because clients depend on it.

A small nit comment is that errBadRequest should really be InvalidArgument to make it clearer for our future selves. Because Internal gets mapped to 500 via gRPC Web, meaning that returning errBadRequest in code results in 500 for the client. That is hard to debug.

@halimi
Copy link
Contributor Author

halimi commented Jun 14, 2024

@johanstokking thank you for clarifying what is the join server and where the error comes from. Now it make more sense.
The error message is already unmarshaled so now I'm checking the message and based on that returning back with a different error code.

I agree to name an internal server error as errBadRequest is a bad idea and makes our life harder later, I've changed it back and created a new errInternalError.

@github-actions github-actions bot added the ui/web This is related to a web interface label Jun 17, 2024
@johanstokking johanstokking marked this pull request as ready for review June 17, 2024 09:11
@halimi halimi self-assigned this Jun 17, 2024
@halimi halimi added this to the v3.31.0 milestone Jun 17, 2024
@halimi halimi merged commit 7c82ae5 into v3.31 Jun 18, 2024
15 checks passed
@halimi halimi deleted the fix/7045-map-error-codes branch June 18, 2024 14:12
@KrishnaIyer
Copy link
Member

@halimi: For future reference, we don't use Closes for issues, unless they don't require testing and/or are small fixes. this way the issue stays open and we only close it after it's tested in staging.

@halimi
Copy link
Contributor Author

halimi commented Jun 20, 2024

@halimi: For future reference, we don't use Closes for issues, unless they don't require testing and/or are small fixes. this way the issue stays open and we only close it after it's tested in staging.

Understood, I will keep in mind, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KrishnaIyer KrishnaIyer KrishnaIyer approved these changes

@johanstokking johanstokking johanstokking approved these changes

@nicholaspcr nicholaspcr Awaiting requested review from nicholaspcr nicholaspcr is a code owner automatically assigned from TheThingsNetwork/stack-reviewers-13

Assignees

@halimi halimi

Labels
ui/web This is related to a web interface
Projects
None yet
Milestone
v3.31.0
Development

Successfully merging this pull request may close these issues.

Map error codes from The Things Join Server
3 participants
@halimi @johanstokking @KrishnaIyer