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

Fix Internal Server Error When Filtering by String Column but the value can be interpreted as another type #36

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Antonio171003
Copy link
Contributor

@Antonio171003 Antonio171003 commented Sep 4, 2024

Fixed an Internal Server Error, Caused when attempting to filter by a column is citext or similar, but the value can be interpreted as another type.

Copy link
Contributor

@AlexITC AlexITC left a comment

Choose a reason for hiding this comment

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

Please add tests

@Antonio171003 Antonio171003 changed the title Fix Internal Server Error When Filtering by Non-String Column Fix Internal Server Error When Filtering by String Column but the value can be interpreted as another type Sep 5, 2024
@@ -532,6 +532,106 @@ class AdminControllerSpec extends PlayPostgresSpec with AdminUtils {
.futureValue
response.headOption.isEmpty must be(true)
}

"don't fail when the column is citext or similar, but the value can be interpreted as Int." in withApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

When the value is CITEXT we should interpret it as string, any reason to interpret it as int?

Copy link
Contributor Author

@Antonio171003 Antonio171003 Sep 7, 2024

Choose a reason for hiding this comment

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

Before, if the value was "123", it was assumed that the value was an Int, and the same for the column type. This caused an internal server error because it compared a citext with an Int.

Same for the others comments

Copy link
Contributor

Choose a reason for hiding this comment

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

From my point of view, the input argument must be handled like the type derived from the database instead of guessing its format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the changes in this pull request consider both the column type and the value to avoid this.

email must be(emailValue)
}

"don't fail when the column is citext or similar, but the value can be interpreted as Decimal." in withApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

email must be(emailValue)
}

"don't fail when the column is citext or similar, but the value can be interpreted as Date." in withApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

email must be(emailValue)
}

"don't fail when the column is citext or similar, but the value can be interpreted as UUID." in withApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

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.

2 participants