Fix edge case in PutMappingRequestTests #37665
Conversation
The recently introduced client side PutMappingRequestTests has an edge case that leads to failing tests. When the "source" or the original request is `null` it gets rendered to xContent as an empty object, which by the test class itself is parsed back as an empty map. For this reason the original and the parsed request differ (one has an empty source, one an empty map). This fixes this edge case by assuming an empty map means a null source in the request. In practice the distinction doesn't matter because all the client side request does is write itself to xContent, which gives the same result regardless of whether `source` is null or empty. Closes elastic#37654
cbuescher
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
| @@ -47,7 +46,10 @@ protected PutMappingRequest createTestInstance() { | |||
| @Override | |||
| protected PutMappingRequest doParseInstance(XContentParser parser) throws IOException { | |||
| PutMappingRequest request = new PutMappingRequest(); | |||
| request.source(parser.map()); | |||
| Map<String, Object> map = parser.map(); | |||
There was a problem hiding this comment.
I was wondering -- would it be a bit unintuitive for a user if they specified a non-null source, but then when going to inspect it, it is null? We could instead only modify the tests (perhaps always specifying a non-null source in createTestInstance, and adding a separate test to verify the null source behavior).
There was a problem hiding this comment.
As far as I understand this parsing code only lives in the test? With the new client-side request that differ from the server side one (which eventually can be fully internal adfter the transport client removal) the parsing is done on the server side, so the user will always see their null-source still. Its only replaced by an empty object when sent.
To the contrary, when working on this fix got to the point where I think that we are overtesting these simple client-side POJOs (they are more or less structs with a toXContent and some validation methods). Having to dig around in AbstractXContentTestCase made me think which parts of it we really need. I'd really be in favour of making these simple ESTestCases, skip most of the randomization and only check all the possible code paths in the xContent output.
There was a problem hiding this comment.
always specifying a non-null source in
createTestInstance
Adding to this, I think the occasional null source is the only useful variant in this test at the moment since it checks that toXContent doesn't blow up on it.
There was a problem hiding this comment.
Ah you're right, my original comment doesn't actually make sense. I hadn't yet had my morning coffee :)
I also like the idea of making this a simple ESTestCase -- are you planning to do this in the current PR? I'm also happy to merge this now, and improve this test in a separate PR if you'd prefer that.
There was a problem hiding this comment.
are you planning to do this in the current PR
No, I wasn't but I can sure do this as a follow up.
|
@jtibshirani I left an answer to your question. Do you think this is ready to be merged then? |
|
Thanks again @cbuescher for fixing this, and sorry to have held it up a bit! |
…ead-de-duplication * elastic/master: (24 commits) [TEST] Mute MlMappingsUpgradeIT testMappingsUpgrade Streamline skip_unavailable handling (elastic#37672) Only bootstrap and elect node in current voting configuration (elastic#37712) Ensure either success or failure path for SearchOperationListener is called (elastic#37467) Target only specific index in update settings test Add a note how to benchmark Elasticsearch Don't use Groovy's `withDefault` (elastic#37726) Adapt SyncedFlushService (elastic#37691) Mute FilterAggregatorTests#testRandom Switch mapping/aggregations over to java time (elastic#36363) [ML] Update ML results mappings on process start (elastic#37706) Modify removal_of_types.asciidoc (elastic#37648) Fix edge case in PutMappingRequestTests (elastic#37665) Use new bulk API endpoint in the docs (elastic#37698) Expose sequence number and primary terms in search responses (elastic#37639) Remove LicenseServiceClusterNotRecoveredTests (elastic#37528) Migrate SpecificMasterNodesIT to Zen2 (elastic#37532) Fix MetaStateFormat tests Use plain text instead of latexmath Fix a typo in a warning message in TestFixturesPlugin (elastic#37631) ...
The recently introduced client side PutMappingRequestTests has an edge case that
leads to failing tests. When the "source" or the original request is
nullitgets rendered to xContent as an empty object, which by the test class itself is
parsed back as an empty map. For this reason the original and the parsed request
differ (one has an empty source, one an empty map). This fixes this edge case by
assuming an empty map means a null source in the request. In practice the
distinction doesn't matter because all the client side request does is write
itself to xContent, which gives the same result regardless of whether
sourceis null or empty.
Closes #37654