isaric commented on code in PR #5311: URL: https://github.com/apache/solr/pull/5311#discussion_r4522302881
########## changelog/unreleased/SOLR-18612-solrj-simpleorderedmap.yml: ########## @@ -0,0 +1,13 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: > + SolrJ: NamedLists that never carry repeated keys (Schema API request bodies, parsed JSON responses, + pivot facets, routed-update bookkeeping) are now created as SimpleOrderedMap. No wire format + changes. Review Comment: I reworded the entry so it no longer reads as if parsed server responses were involved; "parsed JSON responses" was my phrase for the client-side objects JsonMapResponseParser builds. For the record, I couldn't verify that this slice changes any response: the 23 SchemaRequest sites are request bodies (SchemaRequestBodyTest pins them byte-for-byte), and the other four are objects built on the client side, including the one server-side user of JsonMapResponseParser (SchemaDesignerConfigSetHelper), which reads a value out of the parsed object and doesn't embed it. If there is a path from one of these objects into a server response that I'm missing, I'd like to know so I can state it in the entry. The response-side sites are in solr/core, the next slice. ########## changelog/unreleased/SOLR-18612-solrj-simpleorderedmap.yml: ########## @@ -0,0 +1,13 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: > + SolrJ: NamedLists that never carry repeated keys (Schema API request bodies, parsed JSON responses, + pivot facets, routed-update bookkeeping) are now created as SimpleOrderedMap. No wire format + changes. +type: other Review Comment: Done. ########## changelog/unreleased/SOLR-18612-solrj-simpleorderedmap.yml: ########## @@ -0,0 +1,13 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc + +title: > + SolrJ: NamedLists that never carry repeated keys (Schema API request bodies, parsed JSON responses, + pivot facets, routed-update bookkeeping) are now created as SimpleOrderedMap. No wire format + changes. +type: other Review Comment: I've held off adding one for this slice, since I couldn't verify a response change to describe (see above); the core slice will get the note for the json.nl-dependent output changes. If you'd rather the note go in now, tell me what it should say and I'll add it. ########## solr/solrj/src/java/org/apache/solr/client/solrj/request/schema/SchemaRequest.java: ########## @@ -93,39 +94,45 @@ private static NamedList<Object> createAddFieldTypeNamedList( } Map<String, Object> similarityAttributes = fieldTypeDefinition.getSimilarity(); if (similarityAttributes != null && !similarityAttributes.isEmpty()) { - addFieldTypeNamedList.add("similarity", new NamedList<>(similarityAttributes)); + addFieldTypeNamedList.add("similarity", toSimpleOrderedMap(similarityAttributes)); } return addFieldTypeNamedList; } private static NamedList<Object> createAnalyzerNamedList(AnalyzerDefinition analyzerDefinition) { - NamedList<Object> analyzerNamedList = new NamedList<>(); + NamedList<Object> analyzerNamedList = new SimpleOrderedMap<>(); Map<String, Object> analyzerAttributes = analyzerDefinition.getAttributes(); if (analyzerAttributes != null) analyzerNamedList.addAll(analyzerAttributes); List<Map<String, Object>> charFiltersAttributes = analyzerDefinition.getCharFilters(); if (charFiltersAttributes != null) { List<NamedList<Object>> charFiltersList = new ArrayList<>(); for (Map<String, Object> charFilterAttributes : charFiltersAttributes) - charFiltersList.add(new NamedList<>(charFilterAttributes)); + charFiltersList.add(toSimpleOrderedMap(charFilterAttributes)); analyzerNamedList.add("charFilters", charFiltersList); } Map<String, Object> tokenizerAttributes = analyzerDefinition.getTokenizer(); if (tokenizerAttributes != null) { - analyzerNamedList.add("tokenizer", new NamedList<>(tokenizerAttributes)); + analyzerNamedList.add("tokenizer", toSimpleOrderedMap(tokenizerAttributes)); } List<Map<String, Object>> filtersAttributes = analyzerDefinition.getFilters(); if (filtersAttributes != null) { List<NamedList<Object>> filtersList = new ArrayList<>(); for (Map<String, Object> filterAttributes : filtersAttributes) - filtersList.add(new NamedList<>(filterAttributes)); + filtersList.add(toSimpleOrderedMap(filterAttributes)); analyzerNamedList.add("filters", filtersList); } return analyzerNamedList; } + private static SimpleOrderedMap<Object> toSimpleOrderedMap(Map<String, Object> attributes) { Review Comment: Right; the maps are now passed straight through, the helper is gone, and the char filter / filter lists are the definition's own lists of maps. Body test still byte-for-byte identical. ########## solr/solrj/src/test/org/apache/solr/client/solrj/request/schema/SchemaRequestBodyTest.java: ########## @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.client.solrj.request.schema; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.apache.solr.SolrTestCase; +import org.apache.solr.common.util.Utils; +import org.junit.Test; + +/** Pins the JSON bodies that {@link SchemaRequest.Update} requests send to the Schema API. */ +public class SchemaRequestBodyTest extends SolrTestCase { + + @Test + public void testAddFieldTypeBodyMatchesPlainMaps() throws IOException { + FieldTypeDefinition fieldType = new FieldTypeDefinition(); + fieldType.setAttributes(map("name", "text_test", "class", "solr.TextField")); + AnalyzerDefinition analyzer = new AnalyzerDefinition(); + analyzer.setAttributes(map("type", "index")); + analyzer.setCharFilters(List.of(map("class", "solr.HTMLStripCharFilterFactory"))); + analyzer.setTokenizer(map("class", "solr.StandardTokenizerFactory")); + analyzer.setFilters( + List.of( + map("class", "solr.LowerCaseFilterFactory"), + map("class", "solr.ASCIIFoldingFilterFactory"))); + fieldType.setIndexAnalyzer(analyzer); + fieldType.setSimilarity(map("class", "solr.BM25SimilarityFactory", "k1", 1.2)); + + Map<String, Object> expected = + map( + "add-field-type", + map( + "name", + "text_test", + "class", + "solr.TextField", + "indexAnalyzer", + map( + "type", + "index", + "charFilters", + List.of(map("class", "solr.HTMLStripCharFilterFactory")), + "tokenizer", + map("class", "solr.StandardTokenizerFactory"), + "filters", + List.of( + map("class", "solr.LowerCaseFilterFactory"), + map("class", "solr.ASCIIFoldingFilterFactory"))), + "similarity", + map("class", "solr.BM25SimilarityFactory", "k1", 1.2))); + + assertEquals(json(expected), body(new SchemaRequest.AddFieldType(fieldType))); + } + + @Test + public void testAddFieldBodyMatchesPlainMaps() throws IOException { + Map<String, Object> attributes = map("name", "f1", "type", "string", "stored", true); + assertEquals(json(map("add-field", attributes)), body(new SchemaRequest.AddField(attributes))); + } + + @Test + public void testMultiUpdateRepeatsCommandKeys() throws IOException { + SchemaRequest.MultiUpdate multiUpdate = + new SchemaRequest.MultiUpdate( + List.of( + new SchemaRequest.AddField(map("name", "f1", "type", "string")), + new SchemaRequest.AddField(map("name", "f2", "type", "string")), + new SchemaRequest.DeleteField("f0"))); + String body = body(multiUpdate); + + // The Schema API takes several commands of the same kind in one object, so the body must keep + // the repeated keys instead of collapsing them as a Map would. + assertEquals(2, count(body, "\"add-field\":")); + assertEquals( + "{" + + entry("add-field", map("name", "f1", "type", "string")) + + "," + + entry("add-field", map("name", "f2", "type", "string")) + + "," + + entry("delete-field", map("name", "f0")) + + "}", Review Comment: Done with SolrTestCase.assertJSONEquals and text blocks for the two request bodies. The MultiUpdate case stays a text comparison on purpose: assertJSONEquals parses both sides, and a parsed object can't hold the repeated "add-field" key that test exists to check. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
