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

taxa2lca can accept a vector of ints or a vector of strings as input #57

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tibvdm
Copy link
Collaborator

@tibvdm tibvdm commented Nov 28, 2024

The taxa2lca endpoint previously accepted a vector of u32 as input. This PR extends this input field so it can also accept a vector of strings that will be parsed to integers

@tibvdm tibvdm requested a review from pverscha November 28, 2024 10:56
Copy link
Member

@pverscha pverscha left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I've reviewed it and it works as long as all the inputs are of the same type. So if someone enters only integers it works, or if they online provide strings it works. But whenever the data types of the input array are mixed, it doesn't work anymore and I get this error message:

Failed to deserialize the JSON body into the target type: input: data did not match any variant of untagged enum EitherVec at line 3 column 1

Could you take a look at this as well? :) Thanks!

@bmesuere
Copy link
Member

Do we want to accept mixed input? I'm not against it, but if it comes at a performance cost we should think twice about it.

@pverscha
Copy link
Member

pverscha commented Dec 3, 2024

@bmesuere Maybe not, but in that case I think the error message could be improved and should return an HTTP 422 instead :)

@tibvdm Do you think this will cause an extra performance cost? If so, we only need to update the error message.

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