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

Give better error message if record not found #44

Closed
lazappi opened this issue Oct 15, 2024 · 6 comments · Fixed by #70
Closed

Give better error message if record not found #44

lazappi opened this issue Oct 15, 2024 · 6 comments · Fixed by #70
Assignees
Labels
enhancement New feature or request

Comments

@lazappi
Copy link
Collaborator

lazappi commented Oct 15, 2024

If possible with the API, give a different error if a request is successful but a record is not in the database, versus if a request fails completely.

@lazappi lazappi added the enhancement New feature or request label Oct 15, 2024
@rcannood
Copy link
Collaborator

For instance, this is what we get from the API at the moment:

> artifact <- db$Artifact$get("KBW89Mf7IGcekja2hADu")
Error in `private$process_response()`:
! list index out of range

I'd say this is up to the API to do better error handling to provide more descriptive error messages.

For instance, in this case the API should be something like Artifact "KBW89Mf7IGcekja2hADu" not found.

We could implement functionality to map the most common error messages returned by the API into something that is understandable from the perspective of an R user, but at this point it seems the API is simply returning whatever goes wrong inside of the Python code, so there is no knowing which error messages the API might return.

@falexwolf
Copy link
Member

I agree. @fredericenard can comment how fast this can be done.

@fredericenard
Copy link

I did this here for the get record endpoint.

@falexwolf
Copy link
Member

Thanks for resolving, Fred!

Just: Robrecht and Luke won't see what you did because they don't have access to the laminhub repo

@fredericenard
Copy link

@rcannood @lazappi it has been deployed in prod, you should now have a 404 error for non existing records.

@lazappi
Copy link
Collaborator Author

lazappi commented Oct 31, 2024

I just checked with the current main branch and we get this error output:

artifact <- db$Artifact$get("FAKEID")
#> Error in `private$process_response()` at laminr/R/InstanceAPI.R:109:7:
#> ! Failed to get record from instance
#> ℹ Details: 404: Record not found.

Which I think is the result of the API changes and some fixes we made to how the response is processed. I think this is ok but I'll add the status code and a test for this case.

This isn't the case for everything though. For example, if you put nonsense in the search field the API doesn't tell you anything about what the issue is:

api$get_record("core", "artifact", "mePviem4DGM4SFzvLXf3", select = "foo")
#> Error in `private$process_response()` at laminr/R/InstanceAPI.R:109:7:
#> ! Failed to get record from instance with status code 500
#> ℹ Details: 'foo'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants