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

Query Builder fails on a ManyToMany relationship that has a model with an ID that is cast to an Enum #49735

Closed
saadsidqui opened this issue Jan 18, 2024 · 7 comments

Comments

@saadsidqui
Copy link
Contributor

saadsidqui commented Jan 18, 2024

Laravel Version

10.10

PHP Version

8.2.14

Database Driver & Version

PostgreSQL 15.1 (Ubuntu 15.1-1.pgdg20.04+1)

Description

A ManyToMany relationship in which one or more parties casts the id column to an Enum will cause Illuminate\Database\Query\Builder::whereIntegerInRaw() to throw an ErrorException because it tries to cast the (already cast to an enum) id on the model to an int. However, Enums are NOT castable in PHP (I tried php 8.1 up to 8.3).

Steps To Reproduce

A minimal reproducible example is available here.

In the MRE repo are three models: User, Role, and a pivot RoleUser.
A regular ManyToMany relationship is defined between User and Role through the RoleUser pivot.
Role's id column is an integer column that is cast to the backed enum App\Enum\Roles.
Similarly, inside the pivot RoleUser, role_id is also cast to the same enum.

With this setup, running the following query Role::with('users')->get(); throws an ErrorException with the message "Object of class App\Enums\Roles could not be converted to int", in Illuminate\Database\Query\Builder::whereIntegerInRaw(), line 1164.

EDIT:
The fact that the concerned column is autoincrement or not has no incidence on the issue. I have updated various sections of this issue to reflect this.

@crynobone
Copy link
Member

Hey there, thanks for reporting this issue.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here?

Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

@saadsidqui
Copy link
Contributor Author

saadsidqui commented Jan 21, 2024

Sorry for the delay. I'm unfortunately unable to use laravel/installer right now. I will update this later if I get into an environment where I can use it.

In the meantime, I did my best to revamp the minimum reproducible example I had already provided so that things are better separated. I have also updated the description of this issue so that it is more in line with the MRE repo. After checking out the proper commits (see below), all that is needed is a php artisan migrate:fresh --seed and php artisan serve.

  1. For the original issue reported, please see commit c100f25
  2. For a second, seemingly related issue (described below), Please see this commit f4d03ff

In both cases, for a quick start, please look at app/Http/Controllers/TestController.php.

The first issue

In the MRE repo are three models: User, Role, and a pivot RoleUser.
A regular ManyToMany relationship is defined between User and Role through the RoleUser pivot.
Role's id column is an integer column that is cast to the backed enum App\Enum\Roles.
Similarly, inside the pivot RoleUser, role_id is also cast to the same enum.

With this setup, running the following query Role::with('users')->get(); throws an ErrorException with the message "Object of class App\Enums\Roles could not be converted to int", in Illuminate\Database\Query\Builder::whereIntegerInRaw(), line 1164.

The second issue

In the MRE repo, the model Permission has a BelongsTo relationship to the model Role.
Role's id column is an integer column that is cast to the backed enum App\Enum\Roles.
Similarly, inside Permission, role_id is also cast to the same enum.

With this setup, running the following query Permission::with('role')->get(); throws an Error with the message "Object of class App\Enums\Roles could not be converted to string", in Illuminate\Database\Eloquent\Relations\BelongsTo::getEagerModelKeys(), line 140.

It's notable that removing the cast for role_id inside Permission makes the exception go away.

@driesvints
Copy link
Member

Thanks for your report! We'd love to see a PR for this.

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@saadsidqui
Copy link
Contributor Author

saadsidqui commented Jan 23, 2024

Hello,

I submitted PR #49787 . The PR description describes what it fixes.

This PR should fix the two issues at hand; BUT, I have suspicions that there are more places where BackedEnums are not properly handled. I have tried to find some more in the couple of hours I had to familiarize myself with the code. One that stood out to me is getRelatedKeyFrom, which seems to me should also check if the retrieved key has been cast to a BackedEnum to return the proper value, however, I'm not completely sure. Your input would be appreciated.

In general, my familiarity with the code is pretty limited. I would very much like to contribute some more, so any pointers on the following are very much appreciated :

  • How would one go about finding other instances of BackedEnum casts being mishandled?
  • If any other tests/changes should have been included in the PR.
  • How do you usually go about porting these changes to the next version of Laravel so they're not lost ?

Thank you.

@saadsidqui saadsidqui changed the title Query Builder fails on a ManyToMany relationship that has a model with non-auto-increment ID that is also cast to an Enum Query Builder fails on a ManyToMany relationship that has a model with an ID that is cast to an Enum Jan 23, 2024
@driesvints
Copy link
Member

@sidquisaad I'd approach this with waiting until someone asks for support for other portions of the framework.

taylorotwell pushed a commit that referenced this issue Apr 15, 2024
… BackedEnums (#49787)

- Illuminate\Database\Eloquent\Relations\BelongsTo::getForeignKeyFrom() handles BackedEnums properly

See #49735

Co-authored-by: Saad <25284407+sidquisaad@users.noreply.github.com>
@driesvints
Copy link
Member

We merged the PR for this one. Will be tagged today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants