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:Fix static property data bleed in RulesValidation #90

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BadJacky
Copy link
Contributor

Resolve issue where static properties in RulesValidation Trait are shared across multiple classes, causing data bleeding. Added reset method for static properties.

When using the RulesValidation Trait, static properties such as *ValidationRules and *ValidationMessages are shared across multiple classes. This can lead to data bleeding, especially when a Model inherits from another Model that uses the Lift Trait and multiple database save operations are triggered in the same request.

Solution

To resolve this issue, the resetValidations method has been added to the RulesValidation Trait. This method resets the static properties to their initial state before each operation, preventing data bleeding between different Model instances.

Changes

  • Added resetValidations method to the RulesValidation Trait.
  • Updated all relevant Model calls to invoke resetValidations where necessary to ensure data integrity.

This pull request includes changes to improve the consistency and maintainability of the codebase by replacing self with static in various methods and adding a new method to reset validation messages.

Improvements to method calls:

  • src/Lift.php: Replaced all instances of self with static in the bootLift, syncOriginal, ignoredProperties, and fillProperties methods to ensure proper method resolution in subclasses. [1] [2] [3] [4] [5] [6]

New functionality:

This refactoring enhances the flexibility and reusability of the code by allowing subclasses to override static methods more effectively.

Resolve issue where static properties in RulesValidation Trait
are shared across multiple classes, causing data bleeding.
Added reset method for static properties.
@WendellAdriel
Copy link
Owner

Hey @BadJacky thanks for the contribution, but I have a question

What's the data bleed issue here? I'm asking because the static properties related to rules should be shared to avoid the need to build them every time, this was done in order to improve the performance.

Can you show me where and how the data bleed is happening?
Also, can you add some benchmarks before and after this change to understand the impact in the performance?
Because I'm really worried that doing this reset on every action will have impact when doing a lot of DB actions.

@BadJacky
Copy link
Contributor Author

Hey @BadJacky thanks for the contribution, but I have a question

What's the data bleed issue here? I'm asking because the static properties related to rules should be shared to avoid the need to build them every time, this was done in order to improve the performance.

Can you show me where and how the data bleed is happening? Also, can you add some benchmarks before and after this change to understand the impact in the performance? Because I'm really worried that doing this reset on every action will have impact when doing a lot of DB actions.

I have multiple Model files, all of which inherit from my custom BaseModel. I'm using a "Lift Trait" in BaseModel. When I trigger the saving of multiple models within a single request, the validation data retrieved during the second save still contains the data from the first save. To address this, I've implemented a reset method to reset the data.

@BadJacky
Copy link
Contributor Author

Hi @WendellAdriel
I have utilized Pest to debug this bug and have identified a reproducible method. I will provide the corresponding code and a test report for your review. If you have time, please take a look.
The BaseModel
CleanShot 2024-11-12 at 09 19 42@2x
The Book Model
CleanShot 2024-11-12 at 09 20 09@2x
The PostTimestamp Model
CleanShot 2024-11-12 at 09 20 31@2x

The Lift Trait with debug
CleanShot 2024-11-12 at 09 21 42@2x

The Test script
CleanShot 2024-11-12 at 09 21 09@2x

The TestCase output :
CleanShot 2024-11-12 at 09 22 17@2x

I have provider the files I used, you can get details from the files
code.zip

@BadJacky
Copy link
Contributor Author

@WendellAdriel
I would like to ask if you have time to take a look?

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