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

Compare raw attributes only when ensuring uniqueness of paginated related models #2540

Merged
merged 5 commits into from
May 1, 2024

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Apr 9, 2024

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Right now, toJson is used which "is recursive, so all attributes and relations will be converted to JSON" (see https://laravel.com/docs/11.x/eloquent-serialization#serializing-to-json). This means that all accessors are executed as well which can affect the performance of an application. The new approach simply compares the raw attributes of the models which should be enough in almost all cases as far as I can see.

Breaking changes

There might be a tiny chance that something doesn't work as before because accessors are not compared anymore.

@jaulz jaulz force-pushed the fix/compare-raw-attributes-only branch from 963bd3a to 3bde0a2 Compare April 9, 2024 07:55
@jaulz jaulz changed the title fix: compare raw attributes only Compare raw attributes only Apr 9, 2024
@spawnia spawnia added the performance Fix or optimization of performance label Apr 21, 2024
@spawnia spawnia changed the title Compare raw attributes only Compare raw attributes only when ensuring uniqueness of paginated related models May 1, 2024
@spawnia spawnia merged commit dd2b859 into nuwave:master May 1, 2024
31 of 35 checks passed
@skaesdorf
Copy link
Contributor

skaesdorf commented May 2, 2024

@spawnia Hi, this breaks when Models have native Enum casted attributes. $model->getOriginal() casts the attributes. json_encode($relatedModel->getOriginal()) will then fail because enums are not stringable. Can we just use $model->getRawOriginal()?

@jaulz
Copy link
Contributor Author

jaulz commented May 2, 2024

Hm, the best would actually be to pick $original from the model but it's protected.

@skaesdorf
Copy link
Contributor

That would be $model->getRawOriginal()

@jaulz
Copy link
Contributor Author

jaulz commented May 2, 2024

@skaesdorf great, can you raise a PR for that?

@skaesdorf
Copy link
Contributor

skaesdorf commented May 2, 2024

@jaulz Done. See #2550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Fix or optimization of performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants