dsmiley commented on code in PR #5311: URL: https://github.com/apache/solr/pull/5311#discussion_r4520530295
########## 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: ```suggestion type: changed ``` Definitely not "other" as we are changing Solr's API in a backwards incompatible way for some users that use JSON. ########## 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: why convert a Map to a SOM? A Map is already preferrable if we already have it. ########## 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: I think we have an assertJsonEquals or something like that in our tests you can use, which would be nicer to use with a multi-line JSON string instead of all these entry/map method calls. ########## 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: This PR changes Solr's JSON response format for these APIs to use a simple keyed object style instead of arrays of alternating names & values. Say that. It's fine to also mention SimpleOrderedMap but that's an internal detail that a Solr user typically won't know about. -- 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]
