Skip to content

Throw exception when field type is incorrect #60

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

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

outdooracorn
Copy link
Contributor

@outdooracorn outdooracorn commented Oct 26, 2022

Another late addition to our improvements on error handling (#53).

Fixes the following edge cases:

  • if the "op" field is boolean true, it is interpreted as an "add" operation instead of throwing an exception. This is due to true always matching the first [switch case] (Add::OP).
  • if the "op" field is an Array (list) or Object, it results in a "PHP Notice Array/Object to string conversion" in the [UnknownOperationException]
  • if the "path" or "from" fields are not a string, an exception will be thrown in JsonPatch::apply() rather than JsonPatch::import()

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #60 (f8fcc60) into master (b9da3c5) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   96.85%   96.95%   +0.09%     
==========================================
  Files          14       15       +1     
  Lines         573      591      +18     
==========================================
+ Hits          555      573      +18     
  Misses         18       18              
Impacted Files Coverage Δ
src/InvalidFieldTypeException.php 100.00% <100.00%> (ø)
src/JsonPatch.php 99.06% <100.00%> (+0.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@outdooracorn
Copy link
Contributor Author

Realised I didn't explain what this PR addresses! 🙈

We noticed the following edge cases:

  • if the "op" field is boolean true, it is interpreted as an "add" operation instead of throwing an exception. This is due to true always matching the first switch case (Add::OP).
  • if the "op" field is an Array (list) or Object, it results in a "PHP Notice Array/Object to string conversion" in the UnknownOperationException
  • if the "path" or "from" fields are not a string, an exception will be thrown in JsonPatch::apply() rather than JsonPatch::import()

The second commit, makes sure test code coverage doesn't decrease. I suggest we ignore the code climate issue. :)

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Nov 1, 2022
* Update Wikibase from branch 'master'
  to 55ac6e55d1bb900bf4ef549c95599fac16581b81
  - Merge "REST: Handle invalid field type in JSON Patch"
  - REST: Handle invalid field type in JSON Patch
    
    Respond with a more specific error when the JSON Patch contains a field with an invalid type.
    
    Handled in `JsonDiffPatchValidator` while we wait for the upstream PR to be merged.
    swaggest/json-diff#60
    
    Bug: T320705
    Change-Id: I92290f5791b14e28e5fcd87d28c8b26d160e899f
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Nov 1, 2022
Respond with a more specific error when the JSON Patch contains a field with an invalid type.

Handled in `JsonDiffPatchValidator` while we wait for the upstream PR to be merged.
swaggest/json-diff#60

Bug: T320705
Change-Id: I92290f5791b14e28e5fcd87d28c8b26d160e899f
@vearutop vearutop merged commit daa7f05 into swaggest:master Nov 8, 2022
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Nov 14, 2022
* Update Wikibase from branch 'master'
  to d5ac0dea033aa4d6c7f4216e53b7156d32f3c45f
  - Merge "REST: Handle invalid field type via json-diff lib"
  - REST: Handle invalid field type via json-diff lib
    
    Removes temporary solution in JsonDiffJsonPatchValidator in favour of handling the `InvalidFieldTypeException` thrown by the JsonDiff lib after PR#60 merged upstream.
    swaggest/json-diff#60
    
    Bug: T320705
    Change-Id: I7eb29e574964cffa65d470afb8170a238d6827e0
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Nov 14, 2022
Removes temporary solution in JsonDiffJsonPatchValidator in favour of handling the `InvalidFieldTypeException` thrown by the JsonDiff lib after PR#60 merged upstream.
swaggest/json-diff#60

Bug: T320705
Change-Id: I7eb29e574964cffa65d470afb8170a238d6827e0
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