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

Fixed Column not found: 1054 Unknown column '0' in 'field list' [sc-20004] #12599

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented Mar 2, 2023

Description

The API call to Users with method PATCH accepts arrays for the groups parameter. But if the array is weirdly formed the users making the call got an error 500. I found hard to validate that the array is properly formed, since this is in the API and a client could pass whatever value they want... so I use a Try/Catch block to the method causing this error, that way the system doesn't crash and returns the exception as an API error.

Fixes [sc-20004]

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test Configuration:

  • PHP version: 8.2
  • MySQL version: 8.0.31
  • Webserver version: PHP dev server
  • OS version: Debian 11

@what-the-diff
Copy link

what-the-diff bot commented Mar 2, 2023

  • Added try catch block to handle exception when syncing user groups

@snipe
Copy link
Owner

snipe commented Mar 2, 2023

Should we maybe instead initiate a validator to make sure that the ids in the groups array exist in the groups table? It's a bit more code, but would probably be better overall.

@inietov
Copy link
Collaborator Author

inietov commented Mar 3, 2023

I tried with the validator, but to be honest it doesn't change much... I even needed a try/catch to enclose the validator because if I made a PATCH request to the user endpoint with a injected value in the groups column like this:

{
     "groups": [[0,1,2]]
}

The validator still crash. I think this is the best way... since if the array is well formed, and the group passed doesn't exist, the API just ignores the group.

@snipe
Copy link
Owner

snipe commented Mar 3, 2023

What's the error you're seeing when the validator crashes?

@inietov
Copy link
Collaborator Author

inietov commented Mar 3, 2023

image

@snipe
Copy link
Owner

snipe commented Mar 7, 2023

Can we try looping through the array that gets passed and validate on each of the group_ids there, to make sure they exist in the groups table?

It's weird that it's showing as:

{
     "groups": [[0,1,2]]
}

instead of

{
     "groups": [0,1,2]
}

but we should maybe be able to decode that array and make sure the next level down is a set of ids that exist in the departments table.

Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

I'd like to see if we can come up with slightly tighter validation here. Whatever the format that the groups payload should be in, we:

  1. Shouldn't accept anything NOT in that format
  2. Should validate that the groups actually exist, otherwise we could end up with bad data

@snipe snipe marked this pull request as draft March 7, 2023 20:58
@snipe
Copy link
Owner

snipe commented Mar 7, 2023

Converting this to draft so I don't accidentally merge it ;)

@inietov inietov marked this pull request as ready for review March 14, 2023 00:12
@snipe
Copy link
Owner

snipe commented Mar 14, 2023

This looks great - thanks for the extra effort on this to make it even better <3

@snipe snipe merged commit dfd9fcc into snipe:develop Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants