[BUG] Serialize DynamicMapping boolean type

[owaiskazi19]

What is the bug?

With boolean type mapping. Ex(check "dynamic": false):

{
  "dynamic": false,
  "_meta": {
    "schema_version": 5
  },
  "properties": {
    "schema_version": {
      "type": "integer"
    },
    "name": {
      "type": "text",

java client returns

org.opensearch.client.json.UnexpectedJsonEventException: Unexpected JSON event 'VALUE_FALSE' instead of '[KEY_NAME, VALUE_STRING]'
        at org.opensearch.client.json.JsonpUtils.ensureAccepts(JsonpUtils.java:81)
        at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87)
        at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:85)
        at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:189)
        at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:150)
        at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:88)
        at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:92)
        at org.opensearch.client.json.DelegatingDeserializer$SameType.deserialize(DelegatingDeserializer.java:56)
        at org.opensearch.ad.rest.RestCreateDetectorAction.initAnomalyDetectorIndex(RestCreateDetectorAction.java:170)
        at org.opensearch.ad.rest.RestCreateDetectorAction.handleRequest(RestCreateDetectorAction.java:203)
        at org.opensearch.sdk.handlers.ExtensionsRestRequestHandler.handleRestExecuteOnExtensionRequest(ExtensionsRestRequestHandler.java:66)
        at org.opensearch.sdk.ExtensionsRunner.lambda$startTransportService$5(ExtensionsRunner.java:244)
        at org.opensearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:106)
        at org.opensearch.transport.InboundHandler$RequestHandler.doRun(InboundHandler.java:453)
        at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:806)
        at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
        at java.base/java.lang.Thread.run(Thread.java:832)

This is because boolean values are treated as String.
Converting "dynamic": false -> "dynamic": "false" works though.

How can one reproduce the bug?

Use a mapping file like and create an index using CreateIndexRequest.

What is the expected behavior?

JavaClient should treat boolean values as booleans and the request should be send successfully to create an index.

What is your host/environment?

Operating system, version.

Do you have any screenshots?

If applicable, add screenshots to help explain your problem.

Do you have any additional context?

Add any other context about the problem.

[saratvemulapalli]

@owaiskazi19 the way the parsers work is we read everything as strings first. Thats how the mapping is used as well, it parses the key and value, expect them to be a string.

Do we really need to support false vs "false"

[owaiskazi19]

@owaiskazi19 the way the parsers work is we read everything as strings first. Thats how the mapping is used as well, it parses the key and value, expect them to be a string.

Do we really need to support false vs "false"

Thanks for your insight @saratvemulapalli! The reason to support boolean mapping is because of the current mapping file present in the plugins which has the type boolean. Otherwise, we have to create two mapping files one for the plugin and one for extensions. This for maintaining the consistency across the clients. WDYT

[saratvemulapalli]

Thanks for your insight @saratvemulapalli! The reason to support boolean mapping is because of the current mapping file present in the plugins which has the type boolean. Otherwise, we have to create two mapping files one for the plugin and one for extensions. This for maintaining the consistency across the clients. WDYT

It definitely sounds like a one time change when a plugin moves to extension but anyway thats a totally different conversation which the client doesn't care about.
Trying to understand, this change would not bring in consistency across clients, infact we might have to put in a change where we specifically handle it in opensearch-java.

Also reduce pain, could the plugin mapping be updated to "false" ? Would that work?

[amitgalitz]

If currently we have the capability to support boolean mapping and putting false in our mapping shouldn’t we extend that capability to opensearch-java? Could there scenarios when other users that have their own scripts and other implementation for indexing that have it working now and will suddenly see failures when they use opensearch-java or our sdk . While not a must I think there is advantages to keeping it consistent with what we offer in OS and explain in our docs https://opensearch.org/docs/2.3/opensearch/supported-field-types/boolean/

However in the case for AD, since OS supports both false and "false" currently I think its okay to change this line in AD to not block extension development but also I think just supporting both in opensearch-java is something we should do anyways? Not sure what the effort would be like for that.

[patschl]

As I just stumbled upon this issue and it took me a minute to figure out what's going wrong, I would also like a fix for this as it's highly confusing at first.
If you are still open for this I could propose a fix and open a PR?

[wbeckler]

@patschl What would you propose to be the behavior? Would it be a breaking change?

[patschl]

My proposal would be that both, a string and boolean values would be valid.

I have a pretty simple fix in mind which would not be a breaking change.

I'd like to extend the JsonEnum.Deserializer<T extends JsonEnum> to create a deserializer which would add the true/false JsonParser.Events to the acceptedEvents. This should be the only change necessary for it to work, the deserialization process itself should still work as expected.
This deserializer could then be used for the properties which should also accept booleans.
If I remember correctly this should only be DynamicMapping and Refresh.

Is there any concern with this approach?

[dblock]

@patschl Looks good.