solarium icon indicating copy to clipboard operation
solarium copied to clipboard

JSON requests for Update queries

Open thomascorthals opened this issue 3 years ago • 1 comments

This is my first shot at JSON Update requests. I didn't write any integration tests yet, hoping to get some early feedback first. Can be merged without integration tests IMHO as I kept XML requests as the default for now. I'll add the integration tests later on anyway. Documentation will also follow when I'm sure the implementation is stable.

The choice "between XML and JSON" is implemented as a choice "for a request format" to allow adding other formats in the future. (If anyone ever needs to send CSV requests …)

It's a bit unfortunate that building the raw JSON data still requires string concatenation in the code. There is no data structure in PHP that can be json_encode()d directly to a JSON object with non-unique keys.

I would keep JSON requests opt-in for the first release. Once released, I can use them as an early adopter on a platform that generates 5 to 6 requests/second, including a lot of atomic updates. I don't use nested documents, maybe that's something @jupevi can test as an early adopter?

I made some choices w.r.t. backward compatibility in favour of reducing code complexity.

  1. Boosts are ignored completely because they're no longer supported at index-time since Solr 7 and there is no JSON syntax for them anyway. Using boosts with JSON request could be made to throw a RuntimeException, but that's a pointless performance hit. A user actively switching to JSON requests should already be aware that they don't support boosts.
  2. A RawXml command throws the generic exception for an unsupported command. It could be made to throw a specific message instructing you to use XML requests, but it should be obvious that you can't mix in XML with JSON requests.

I've kept the unit tests for XML and JSON request builders in sync, even when certain tests are specifically aimed at an implementation detail of one of the formats (e.g. escaping < and > for XML or multivalued DateTimes for JSON).

thomascorthals avatar Sep 22 '22 23:09 thomascorthals

Codecov Report

Base: 97.20% // Head: 97.23% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (a22a821) compared to base (add81df). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
+ Coverage   97.20%   97.23%   +0.03%     
==========================================
  Files         359      360       +1     
  Lines        8833     8913      +80     
==========================================
+ Hits         8586     8667      +81     
+ Misses        247      246       -1     
Flag Coverage Δ
unittests 97.23% <100.00%> (+0.03%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Core/Query/AbstractRequestBuilder.php 100.00% <ø> (ø)
src/QueryType/Update/RequestBuilder/Xml.php 100.00% <ø> (ø)
src/QueryType/Update/Query/Document.php 100.00% <100.00%> (ø)
src/QueryType/Update/Query/Query.php 100.00% <100.00%> (ø)
src/QueryType/Update/RequestBuilder/Json.php 100.00% <100.00%> (ø)
src/Plugin/PrefetchIterator.php 100.00% <0.00%> (+1.69%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Sep 22 '22 23:09 codecov[bot]

@thomascorthals thanks for providing that implementation. I agree to merge it as it is and extend the test coverage later one the merge conflict is solved.

mkalkbrenner avatar Oct 10 '22 07:10 mkalkbrenner