Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

Fix various /*.get endpoints returning error 500 when not provided with id #773

Merged
merged 2 commits into from
Feb 4, 2019

Conversation

unnawut
Copy link
Contributor

@unnawut unnawut commented Feb 1, 2019

Issue/Task Number: #755
Closes #755

Overview

This PR adds input handling for when id is not provided to various /*.get endpoints.

⚠️ This PR add/edit the controller tests. This will require migrating these tests manually to the unified controller tests on master branch.

Changes

  • Add a handle_error(conn, :missing_id) or handle_error(conn, :invalid_parameter, _) to missing controller fallbacks.

Implementation Details

Some older code seems to be missing a fallback for when id is not provided.

Note that there are still some inconsistency in how error messages are returned. Some endpoints would return simply "Invalid parameter provided.", while some others would return "Invalid parameter provided. `id` is required."

But these inconsistencies are a cosmetic issue, since providers should not be parsing the plaintext description and there are a lot of places that can be improved. Will open a separate ticket for this.

Usage

Try, for example, /account.get_wallets without providing an id. It should return a proper error response. E.g.

curl 'http://localhost:4000/api/admin/account.get_wallets' \
-H 'Authorization: OMGAdmin <truncated>' \
-H 'Content-Type: application/json;charset=UTF-8' \
-H 'Accept: application/vnd.omisego.v1+json' \
-d '{"per_page":10,"sort_by":"identifier","sort_dir":"desc","owned":true}'

Impact

No changes to the API specs or DB schema.

@unnawut unnawut added this to the v1.1 milestone Feb 1, 2019
@unnawut unnawut self-assigned this Feb 1, 2019
@ghost ghost added the s2/wip 🚧 label Feb 1, 2019
@unnawut unnawut added s3/review 👀 flag/needs upgrade path 🛠️ This requires an upgrade path e.g. running an extra command or settings change and removed s2/wip 🚧 labels Feb 1, 2019
@unnawut unnawut merged commit 5e04e8a into v1.1 Feb 4, 2019
@unnawut unnawut deleted the 755-no-account-500 branch February 4, 2019 05:07
@ghost ghost removed the s3/review 👀 label Feb 4, 2019
@unnawut unnawut mentioned this pull request Feb 4, 2019
8 tasks
@unnawut unnawut removed the flag/needs upgrade path 🛠️ This requires an upgrade path e.g. running an extra command or settings change label Feb 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants