Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10413

v2 API: parsed JSON type should be coerced to expected type

Details

    • Bug
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • None
    • 7.0
    • v2 API
    • None

    Description

      v2 API request bodies are parsed as JSON and are checked via JSON schema for the correct types. But since the JSON parser used (noggit) accepts a relaxed form of JSON, it is weirdly necessary to quote some things but not others.

      For example, after bin/solr start -e cloud -noprompt, curl http://localhost:8983/v2/cluster -H 'Content-type: application/json' -d '{ set-property: { name: autoAddReplicas, val: false } }' returns:

      <html>
      <head>
      <meta http-equiv="Content-Type" content="text/html;charset=utf-8"/>
      <title>Error 400 {metadata={error-class=org.apache.solr.api.ApiBag$ExceptionWithErrObject,root-error-class=org.apache.solr.api.ApiBag$ExceptionWithErrObject},errorMessages=[{set-property={name=autoAddReplicas, val=false}, errorMessages=[Expected type : string but found : falsein object : false]}],msg=Error in command payload,code=400}</title>
      </head>
      <body><h2>HTTP ERROR 400</h2>
      <p>Problem accessing /solr/____v2/cluster. Reason:
      <pre>    {metadata={error-class=org.apache.solr.api.ApiBag$ExceptionWithErrObject,root-error-class=org.apache.solr.api.ApiBag$ExceptionWithErrObject},errorMessages=[{set-property={name=autoAddReplicas, val=false}, errorMessages=[Expected type : string but found : falsein object : false]}],msg=Error in command payload,code=400}</pre></p>
      </body>
      </html>
      

      By contrast, if I quote the propery value, the request succeeds: curl http://localhost:8983/v2/cluster -H 'Content-type: application/json' -d '{ set-property: { name: autoAddReplicas, val: "false" } }'

      This is annoying because the property is semantically boolean, even though cluster properties' keys and values are always typed String.

      This error occurs because the API spec for the v2 Cluster API's set-property command requires string typed values - from solr/core/src/resources/apispec/cluster.Commands.json:

      {
        "documentation": [...],
        "description": [...],
        "methods": [ "POST" ],
        "url": { "paths": [ "/cluster" ] },
        "commands": {
      [...]
          "set-property": {
            "type": "object",
            "documentation": [...],
            "description": [...],
            "properties": {
              "name": { "type": "string",  "description": [...] },
              "val": { "type": "string", "description": [...] }
      [...]
      

      I'm not sure how wide-spread the problem is, but at a minimum for this particular API (setting a cluster property), Solr should accept both keys and values of any (JSON) type and just toString() their values.

      Attachments

        1. SOLR-10413.patch
          23 kB
          Cao Manh Dat
        2. SOLR-10413.patch
          23 kB
          Noble Paul
        3. SOLR-10413.patch
          24 kB
          Cao Manh Dat

        Activity

          caomanhdat Cao Manh Dat added a comment - - edited

          A patch for this ticket, I've rewritten json validator from scratch. The new json validator :

          • can add more validations easily
          • to solve this ticket problem, the patch changed specification and validator to accept multiple-type values
          caomanhdat Cao Manh Dat added a comment - - edited A patch for this ticket, I've rewritten json validator from scratch. The new json validator : can add more validations easily to solve this ticket problem, the patch changed specification and validator to accept multiple-type values
          noble.paul Noble Paul added a comment -

          let us not reflection here

            String className = Character.toUpperCase(fname.toString().charAt(0))
                    + fname.toString().substring(1) + "Validator";
                Class<Validator> validatorClass = getValidatorClass(fname, className);
                Constructor<Validator> constructor = (Constructor<Validator>) validatorClass.getDeclaredConstructors()[0];
          

          We have a well defined set of Validators . You can keep a Map of fname vs. Validator objects and lookup

          noble.paul Noble Paul added a comment - let us not reflection here String className = Character .toUpperCase(fname.toString().charAt(0)) + fname.toString().substring(1) + "Validator" ; Class <Validator> validatorClass = getValidatorClass(fname, className); Constructor<Validator> constructor = (Constructor<Validator>) validatorClass.getDeclaredConstructors()[0]; We have a well defined set of Validators . You can keep a Map of fname vs. Validator objects and lookup
          caomanhdat Cao Manh Dat added a comment -

          I disagree with using map. I think reflection make

          • the code clean and compact
          • very easy to add new validator
          • we don't have to cast parameter to right type for each validator

          The scope/usage of validator is small enough so the harm of reflection is almost nothing.

          caomanhdat Cao Manh Dat added a comment - I disagree with using map. I think reflection make the code clean and compact very easy to add new validator we don't have to cast parameter to right type for each validator The scope/usage of validator is small enough so the harm of reflection is almost nothing.
          noble.paul Noble Paul added a comment -

          The map usage is type safe and less complex

          noble.paul Noble Paul added a comment - The map usage is type safe and less complex
          caomanhdat Cao Manh Dat added a comment -

          I don't think

          +  static final Map<String, Function<Pair<Map,Object>, Validator>> VALIDATORS= new HashMap<>();
          +  static {
          +    VALIDATORS.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));
          +    VALIDATORS.put("properties", pair -> new PropertiesValidator(pair.first(), (Map) pair.second()));
          +    VALIDATORS.put("type", pair -> new TypeValidator(pair.first(), (Map) pair.second()));
          +    VALIDATORS.put("required", pair -> new RequiredValidator(pair.first(), (List)pair.second()));
          +  }
          

          is more type safe and less complex than reflection?

          caomanhdat Cao Manh Dat added a comment - I don't think + static final Map< String , Function<Pair<Map, Object >, Validator>> VALIDATORS= new HashMap<>(); + static { + VALIDATORS.put( "items" , pair -> new ItemsValidator(pair.first(), (Map) pair.second())); + VALIDATORS.put( "properties" , pair -> new PropertiesValidator(pair.first(), (Map) pair.second())); + VALIDATORS.put( "type" , pair -> new TypeValidator(pair.first(), (Map) pair.second())); + VALIDATORS.put( "required" , pair -> new RequiredValidator(pair.first(), (List)pair.second())); + } is more type safe and less complex than reflection?
          noble.paul Noble Paul added a comment - - edited

          is more type safe and less complex than reflection?

          yes. How do you know that such a class exist or if that class is indeed a Validator when you use reflection. In this case you are sure about it

          noble.paul Noble Paul added a comment - - edited is more type safe and less complex than reflection? yes. How do you know that such a class exist or if that class is indeed a Validator when you use reflection. In this case you are sure about it
          caomanhdat Cao Manh Dat added a comment -

          Updated patch, without reflection + oneOf validator. Will commit this patch soon.

          caomanhdat Cao Manh Dat added a comment - Updated patch, without reflection + oneOf validator. Will commit this patch soon.

          Commit c93409392d780892542ba736739099970a26632f in lucene-solr's branch refs/heads/master from caomanhdat
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c934093 ]

          SOLR-10413: v2 API: parsed JSON type should be coerced to expected type

          jira-bot ASF subversion and git services added a comment - Commit c93409392d780892542ba736739099970a26632f in lucene-solr's branch refs/heads/master from caomanhdat [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c934093 ] SOLR-10413 : v2 API: parsed JSON type should be coerced to expected type

          Commit 38205d7adab7fc184d510f1fa5af631699f75836 in lucene-solr's branch refs/heads/master from caomanhdat
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=38205d7 ]

          SOLR-10413: Update CHANGES.txt

          jira-bot ASF subversion and git services added a comment - Commit 38205d7adab7fc184d510f1fa5af631699f75836 in lucene-solr's branch refs/heads/master from caomanhdat [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=38205d7 ] SOLR-10413 : Update CHANGES.txt
          sarowe Steven Rowe added a comment -

          Policeman Jenkins found a reproducing seed for a failure that started with c93409392d, the first commit on this issue (according to git bisect) https://jenkins.thetaphi.de/job/Lucene-Solr-master-Solaris/1304/:

             [junit4] ERROR   0.04s J1 | TestCollectionAPIs.testCommands <<<
             [junit4]    > Throwable #1: java.lang.RuntimeException: Error in command payload[{
             [junit4]    >     "modify":{
             [junit4]    >       "rule":"replica:*,cores:<5",
             [junit4]    >       "autoAddReplicas":false},
             [junit4]    >     "errorMessages":["Value is not valid, expected one of: [ARRAY], found: String"]}]
             [junit4]    > 	at __randomizedtesting.SeedInfo.seed([41BCD0097E4852E2:608019F11472B286]:0)
             [junit4]    > 	at org.apache.solr.handler.admin.TestCollectionAPIs.makeCall(TestCollectionAPIs.java:189)
             [junit4]    > 	at org.apache.solr.handler.admin.TestCollectionAPIs.compareOutput(TestCollectionAPIs.java:148)
             [junit4]    > 	at org.apache.solr.handler.admin.TestCollectionAPIs.testCommands(TestCollectionAPIs.java:120)
             [junit4]    > 	at java.lang.Thread.run(Thread.java:748)
             [junit4]    > Caused by: org.apache.solr.api.ApiBag$ExceptionWithErrObject: Error in command payload
             [junit4]    > 	at org.apache.solr.api.ApiBag.getCommandOperations(ApiBag.java:320)
             [junit4]    > 	at org.apache.solr.handler.admin.TestCollectionAPIs$1.getCommands(TestCollectionAPIs.java:173)
             [junit4]    > 	at org.apache.solr.handler.admin.BaseHandlerApiSupport$1.call(BaseHandlerApiSupport.java:84)
             [junit4]    > 	at org.apache.solr.handler.admin.TestCollectionAPIs.makeCall(TestCollectionAPIs.java:187)
          [...]
             [junit4]   2> NOTE: test params are: codec=Asserting(Lucene70): {}, docValues:{}, maxPointsInLeafNode=1627, maxMBSortInHeap=5.487446584455346, sim=RandomSimilarity(queryNorm=false): {}, locale=is-IS, timezone=America/Monterrey
             [junit4]   2> NOTE: SunOS 5.11 amd64/Oracle Corporation 1.8.0_131 (64-bit)/cpus=3,threads=1,free=59314248,total=224067584
          
          sarowe Steven Rowe added a comment - Policeman Jenkins found a reproducing seed for a failure that started with c93409392d , the first commit on this issue (according to git bisect ) https://jenkins.thetaphi.de/job/Lucene-Solr-master-Solaris/1304/ : [junit4] ERROR 0.04s J1 | TestCollectionAPIs.testCommands <<< [junit4] > Throwable #1: java.lang.RuntimeException: Error in command payload[{ [junit4] > "modify":{ [junit4] > "rule":"replica:*,cores:<5", [junit4] > "autoAddReplicas":false}, [junit4] > "errorMessages":["Value is not valid, expected one of: [ARRAY], found: String"]}] [junit4] > at __randomizedtesting.SeedInfo.seed([41BCD0097E4852E2:608019F11472B286]:0) [junit4] > at org.apache.solr.handler.admin.TestCollectionAPIs.makeCall(TestCollectionAPIs.java:189) [junit4] > at org.apache.solr.handler.admin.TestCollectionAPIs.compareOutput(TestCollectionAPIs.java:148) [junit4] > at org.apache.solr.handler.admin.TestCollectionAPIs.testCommands(TestCollectionAPIs.java:120) [junit4] > at java.lang.Thread.run(Thread.java:748) [junit4] > Caused by: org.apache.solr.api.ApiBag$ExceptionWithErrObject: Error in command payload [junit4] > at org.apache.solr.api.ApiBag.getCommandOperations(ApiBag.java:320) [junit4] > at org.apache.solr.handler.admin.TestCollectionAPIs$1.getCommands(TestCollectionAPIs.java:173) [junit4] > at org.apache.solr.handler.admin.BaseHandlerApiSupport$1.call(BaseHandlerApiSupport.java:84) [junit4] > at org.apache.solr.handler.admin.TestCollectionAPIs.makeCall(TestCollectionAPIs.java:187) [...] [junit4] 2> NOTE: test params are: codec=Asserting(Lucene70): {}, docValues:{}, maxPointsInLeafNode=1627, maxMBSortInHeap=5.487446584455346, sim=RandomSimilarity(queryNorm=false): {}, locale=is-IS, timezone=America/Monterrey [junit4] 2> NOTE: SunOS 5.11 amd64/Oracle Corporation 1.8.0_131 (64-bit)/cpus=3,threads=1,free=59314248,total=224067584

          Commit 9e3c710d0c7d8d82468c4e6a99717b08655db486 in lucene-solr's branch refs/heads/master from caomanhdat
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9e3c710 ]

          SOLR-10413: Fix test failure (with better json validators we can capture invalid input)

          jira-bot ASF subversion and git services added a comment - Commit 9e3c710d0c7d8d82468c4e6a99717b08655db486 in lucene-solr's branch refs/heads/master from caomanhdat [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9e3c710 ] SOLR-10413 : Fix test failure (with better json validators we can capture invalid input)

          Commit eb475db9c4811d6364000e6ebe372773b3585df0 in lucene-solr's branch refs/heads/master from caomanhdat
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb475db ]

          SOLR-10413: Fix precommit

          jira-bot ASF subversion and git services added a comment - Commit eb475db9c4811d6364000e6ebe372773b3585df0 in lucene-solr's branch refs/heads/master from caomanhdat [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eb475db ] SOLR-10413 : Fix precommit

          People

            caomanhdat Cao Manh Dat
            sarowe Steven Rowe
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: