malliaridis commented on code in PR #4329:
URL: https://github.com/apache/solr/pull/4329#discussion_r3323479243
##########
solr/core/src/java/org/apache/solr/jersey/SolrJacksonMapper.java:
##########
@@ -49,12 +52,20 @@ private static ObjectMapper createObjectMapper() {
final SimpleModule customTypeModule = new SimpleModule();
customTypeModule.addSerializer(new NamedListSerializer(NamedList.class));
- return new ObjectMapper()
- // TODO Should failOnUnknown=false be made available on a "permissive"
object mapper instead
- // of the "main" one?
- .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
- .setSerializationInclusion(JsonInclude.Include.NON_NULL)
- .registerModule(customTypeModule);
+ final ObjectMapper mapper =
+ new ObjectMapper()
+ // TODO Should failOnUnknown=false be made available on a
"permissive" object mapper
+ // instead of the "main" one?
+ .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES,
false)
+ .setSerializationInclusion(JsonInclude.Include.NON_NULL)
+ .registerModule(customTypeModule);
+
+ mapper
+ .coercionConfigFor(LogicalType.Textual)
Review Comment:
- [ ] This only covers lenient cases for where text / string is expected.
For cases where a number is expected and a text is provided, this will not
catch a bad type.
- [ ] This approach only covers cases that are listed. For example, I am
allowed to provide `true` instead of a string and it will not fail.
- [ ] When running into this coercion violation, the message could be
optimized by customizing it (if possible). Right now it says:
```
"msg": "Cannot coerce Integer value (123) to `java.lang.String` value
(but could if coercion was enabled using `CoercionConfig`)"
```
##########
solr/core/src/test/org/apache/solr/handler/V2ApiIntegrationTest.java:
##########
@@ -282,6 +282,20 @@ public void testV2ApiErrorHandling() throws Exception {
assertRSECodeAndMessage(ex2, 400, "Could not find collection",
"coll-does-not-exist");
}
+ @Test
+ public void testCreateCollectionWithNumericNameIsRejected() {
+ final V2Request request =
+ new V2Request.Builder("/collections")
+ .withMethod(SolrRequest.METHOD.POST)
+ .withPayload("{\"name\": 123, \"numShards\": 1}")
+ .build();
+
+ final RemoteSolrException ex =
+ expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
+
+ assertEquals(400, ex.code());
+ }
+
Review Comment:
```suggestion
@Test
public void testCreateCollectionWithBooleanNameIsRejected() {
final V2Request request =
new V2Request.Builder("/collections")
.withMethod(SolrRequest.METHOD.POST)
.withPayload("{\"name\": true, \"numShards\": 1}")
.build();
final RemoteSolrException ex =
expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
assertEquals(400, ex.code());
}
@Test
public void testCreateCollectionWithStringNumShardsIsRejected() {
final V2Request request =
new V2Request.Builder("/collections")
.withMethod(SolrRequest.METHOD.POST)
.withPayload("{\"name\": \"stringNumShards_collection\",
\"numShards\": \"1\"}")
.build();
final RemoteSolrException ex =
expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
assertEquals(400, ex.code());
}
@Test
public void testCreateCollectionWithDecimalNumShardsIsRejected() {
final V2Request request =
new V2Request.Builder("/collections")
.withMethod(SolrRequest.METHOD.POST)
.withPayload("{\"name\": \"decimalNumShards_collection\",
\"numShards\": 1.5}")
.build();
final RemoteSolrException ex =
expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
assertEquals(400, ex.code());
}
@Test
public void testCreateCollectionWithStringShuffleNodesIsRejected() {
final V2Request request =
new V2Request.Builder("/collections")
.withMethod(SolrRequest.METHOD.POST)
.withPayload("{\"name\": \"stringShuffleNodes_collection\",
\"shuffleNodes\": \"true\"}")
.build();
final RemoteSolrException ex =
expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
assertEquals(400, ex.code());
}
@Test
public void testCreateCollectionWithNumberShuffleNodesIsRejected() {
final V2Request request =
new V2Request.Builder("/collections")
.withMethod(SolrRequest.METHOD.POST)
.withPayload("{\"name\": \"numberShuffleNodes_collection\",
\"shuffleNodes\": 0}")
.build();
final RemoteSolrException ex =
expectThrows(RemoteSolrException.class, () ->
request.process(cluster.getSolrClient()));
assertEquals(400, ex.code());
}
```
I have added a few more test cases for helping identifying additional cases.
Not sure if I have covered everything. There is one case of decimal number
retrieving an integer, which should pass I believe, but vice-versa should be a
bad request (test added).
--
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]
