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:this->error can be reset in the middle of validateOrThrow #4

Merged
merged 2 commits into from
May 8, 2024

Conversation

jwadhams
Copy link
Collaborator

@jwadhams jwadhams commented May 8, 2024

Caught a problem in production where some times validateOrThrow will throw a type error

"Carsdotcom\\JsonSchemaValidation\\Exceptions\\JsonSchemaValidationException::__construct(): Argument #2 ($error) must be of type Opis\\JsonSchema\\Errors\\ValidationError, null given, called in /var/www/html/vendor/carsdotcom/laravel-json-schema/app/SchemaValidatorService.php on line 141"

I wasn't able to reproduce the problem in a unit test (even using an exact copy of this DealData and an export-import of the Account). I ended up copy-pasting the implementation of validateOrThrow into Tinker on the affected Kubernetes pod, where I was able to reproduce, and experimenting with echo statements and iteratively rewriting.

The problem appears to be that in a complicated app, the Log::debug call can synchronously involve other systems, some of which may also be using this Facade to validate other data. In some roundabout way Log::debug is causing SchemaValidatorService::error to be reset to null -- because SchemaValidatorProvider is intentionally designed to be a singleton that $error property is effectively a global. Yikes!

The fix in Tinker was to get error into a local variable at the top of the method. But I think the right fix for the library is to obsolete the pattern where validate returns a boolean, then the race is on to get the error -- instead I introduced a method that returns the full \Opis\JsonSchema\ValidationResult (both boolean valid plus the errors array, in one bundle) and refactored validateOrThrow to use that method. I also totally obsoleted the idea of getting the error in a separate call, since as a singleton that can't ever be totally safe.

@jwadhams jwadhams changed the title fix: If this->error can be reset in the course of validateOrThrow fix:this->error can be reset in the middle of validateOrThrow May 8, 2024
@jwadhams
Copy link
Collaborator Author

jwadhams commented May 8, 2024

Note to Cars.com team, you can test this in Online Shopper before we merge by editing composer.json like

        "carsdotcom/laravel-json-schema": "dev-hotfix/error-disappears-during-reporting as 1.0.2",

then running

composer upgrade carsdotcom/laravel-json-schema

(You have to use the as syntax or it won't understand how this fulfills the requirements for JsonModel)

@bbene bbene merged commit a84c84d into development May 8, 2024
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