Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Form] Filter non-uuids when selecting entities by guid ID#17494

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
paulferrett wants to merge3 commits intosymfony:2.3frompaulferrett:ticket_17488
Closed

[Form] Filter non-uuids when selecting entities by guid ID#17494

paulferrett wants to merge3 commits intosymfony:2.3frompaulferrett:ticket_17488

Conversation

@paulferrett
Copy link
Contributor

QA
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets17488
LicenseMIT
Doc PR-

This fix prevents the ORMQueryBuilderLoader from passing non-uuid values through to the query builder for a uuid id entity, as they will cause exceptions on some platforms, e.g. Postgres

Copy link
Contributor

Choose a reason for hiding this comment

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

why not plain comparison?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Very good point! I initially (incorrectly) had a check for 'uuid' as well and didn't change to a comparison when I removed that. Will fix.

@Tobion
Copy link
Contributor

I don't think it's the task of the loader to do that as it's magic and unreliable. Instead you should add validation on the form so it does not even get this far. There is even a UUID validator:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/UuidValidator.php

So I'm 👎

@paulferrett
Copy link
ContributorAuthor

@Tobion My thinking was that if it's the job of the loader to check for correct int (or int string) ids because it makes Postgres fail, then it would be it's the loader's job to protect against other invalid data-types to the database which cause the same failure.

I agree the use of the regex is a bit flaky and could be factored out to use the same code as the UUID validator constraint.

I'm very happy to be corrected on this, but I can't get the validator approach to work... The entities seem to be requested from the loader before the validation occurs. So the approach I can think of would be for me to add a pre-submit listener which validates the passed strings as uuids -- which seems bad for a couple of reasons:

  1. It's a lot of boilerplate code
  2. It relies on the creator of the form to know which of the related entities use uuid values for their ids.

For a little bit of background, this is something that broke for me when upgrading to 2.8 from 2.5. In 2.5 all entities were loaded into a choice list and then the loaded array was checked for the given id... in 2.8 the entity loader optimisation means that the id passed to the form is sent directly to the database, which means now Postgres causes an exception when an invalid uuid is submitted with the form, including an empty string when the field is omitted.

@Tobion
Copy link
Contributor

I see. If validation happens after querying the DB, it is problematic. But that sounds broken anyway. Lets just think about another case: Having a maxlength for a string will also not be validated before querying the DB? That could also result in errors when the DB is configured to throw an errror instead of trimming the data. And there are probably other datatypes apart from int and UUID that can cause invalid queries without validating the user input.
So this fix is only a workaround IMO. Also the UUID constrait in the validator is more complex. So I assume this simple regex does not handle all cases. Maybe the DB only validates the syntax but not the contents? Or maybe it depends on the RDBMS.

@paulferrett
Copy link
ContributorAuthor

I agree it's a workaround, but only as much as the existing check in there for int-only ids committed in45579fd -- what do you think@webmozart ?

Yes, the regex is simplistic, e.g. if only one of the dashes or one of the wrapping braces/brackets is missing this will pass.

The other thing that comes to mind as a possible break this will create is using non-valid uuids will probably work if you're using a DB that doesn't have a native uuid type, e.g. MySQL -- so if someone has a data-type of uuid and then just uses some custom id (e.g. "user-xyz123") that might work -- I haven't tested this though.

@fabpot
Copy link
Member

Closing for the reasons given by@Tobion. We should have a solution that covers everything, but having a partial solution for only one use case that is not even full-proof does not seem worthwhile.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@paulferrett@Tobion@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp