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

Enforce required parameters in SolrJ Collection APIs

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.2, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      Several Collection API commands have required parameters. We should make these constructor parameters, to enforce setting these in the API.

      1. SOLR-8765.patch
        56 kB
        Alan Woodward
      2. SOLR-8765.patch
        144 kB
        Alan Woodward
      3. SOLR-8765-revert-behavior.patch
        18 kB
        Anshum Gupta
      4. SOLR-8765-splitshard.patch
        4 kB
        Anshum Gupta
      5. SOLR-8765-splitshard.patch
        4 kB
        Anshum Gupta

        Activity

        Hide
        romseygeek Alan Woodward added a comment -

        Patch.

        This is a backwards-break for the API, but I think that's fine for 6.0?

        Show
        romseygeek Alan Woodward added a comment - Patch. This is a backwards-break for the API, but I think that's fine for 6.0?
        Hide
        gerlowskija Jason Gerlowski added a comment -

        I've been poking around in that code a bit recently, so I thought I might take a look at your patch. It looked good to me.

        The only tweak I'd suggest is that you might be able to move the validateName IAE message String into SolrIdentifierValidator as a constant. Ostensibly, that message is useful for anything that calls into SIV, so I'd say there's value in extracting it to there (rather than having every caller have their own slightly different error message).

        Arguably, that's not related to this JIRA at all though. So take my suggestion with a grain of salt.

        Looks good to me!

        Show
        gerlowskija Jason Gerlowski added a comment - I've been poking around in that code a bit recently, so I thought I might take a look at your patch. It looked good to me. The only tweak I'd suggest is that you might be able to move the validateName IAE message String into SolrIdentifierValidator as a constant. Ostensibly, that message is useful for anything that calls into SIV, so I'd say there's value in extracting it to there (rather than having every caller have their own slightly different error message). Arguably, that's not related to this JIRA at all though. So take my suggestion with a grain of salt. Looks good to me!
        Hide
        romseygeek Alan Woodward added a comment -

        This has missed 6.0, so here's a patch that deprecates the constructors instead. It also:

        • adds some static factory methods to help distinguish between different use-cases for some requests
        • adds a whole bunch of javadoc
        • adds some pre-request validation, so that we can catch badly-formed requests before they're sent to a Solr instance.

        The patch applies on top of SOLR-8782.

        Show
        romseygeek Alan Woodward added a comment - This has missed 6.0, so here's a patch that deprecates the constructors instead. It also: adds some static factory methods to help distinguish between different use-cases for some requests adds a whole bunch of javadoc adds some pre-request validation, so that we can catch badly-formed requests before they're sent to a Solr instance. The patch applies on top of SOLR-8782 .
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 55c595a9dcea7d3426e7dcc2690324624287b204 in lucene-solr's branch refs/heads/master from Alan Woodward
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=55c595a ]

        SOLR-8765: Enforce required parameters in SolrJ Collections API

        Show
        jira-bot ASF subversion and git services added a comment - Commit 55c595a9dcea7d3426e7dcc2690324624287b204 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=55c595a ] SOLR-8765 : Enforce required parameters in SolrJ Collections API
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit c1277cda1116a44b3371a3fa8364cc2032e14273 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c1277cd ]

        SOLR-8765: Enforce required parameters in SolrJ Collections API

        Show
        jira-bot ASF subversion and git services added a comment - Commit c1277cda1116a44b3371a3fa8364cc2032e14273 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c1277cd ] SOLR-8765 : Enforce required parameters in SolrJ Collections API
        Hide
        romseygeek Alan Woodward added a comment -

        Thanks for the review Jason!

        Show
        romseygeek Alan Woodward added a comment - Thanks for the review Jason!
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-8765: Throw SolrException rather than IAE on name validation

        Show
        jira-bot ASF subversion and git services added a comment - Commit f24810bdf1e8b1949970ce743373794e0b1ffc96 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f24810b ] SOLR-8765 : Throw SolrException rather than IAE on name validation
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 8b408be7a2960a5f5a371cc6acb5af3e15b31344 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8b408be ]

        SOLR-8765: Throw SolrException rather than IAE on name validation

        Show
        jira-bot ASF subversion and git services added a comment - Commit 8b408be7a2960a5f5a371cc6acb5af3e15b31344 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8b408be ] SOLR-8765 : Throw SolrException rather than IAE on name validation
        Hide
        gerlowskija Jason Gerlowski added a comment - - edited

        IMO, Id rather have SolrIdentifierValidator return boolean than SolrException. SolrException is intended to be used on the server side, as it can be converted into a status code. That aspect of the exception doesn't make a ton of sense being used on the client/Solr J side.

        (Disclaimer, when I first wrote SolrIdentifierValidator, I had it throwing SolrException. I was then converted after talking to Anshum about the downsides of using it client-side)

        Show
        gerlowskija Jason Gerlowski added a comment - - edited IMO, Id rather have SolrIdentifierValidator return boolean than SolrException. SolrException is intended to be used on the server side, as it can be converted into a status code. That aspect of the exception doesn't make a ton of sense being used on the client/Solr J side. (Disclaimer, when I first wrote SolrIdentifierValidator, I had it throwing SolrException. I was then converted after talking to Anshum about the downsides of using it client-side)
        Hide
        anshumg Anshum Gupta added a comment -

        Alan Woodward Having the validator return a boolean value seem much better to me than to return the input value in case of successful validation. What was the reason behind changing that behavior.

        Show
        anshumg Anshum Gupta added a comment - Alan Woodward Having the validator return a boolean value seem much better to me than to return the input value in case of successful validation. What was the reason behind changing that behavior.
        Hide
        romseygeek Alan Woodward added a comment -

        Everywhere that was actually using the validator had exactly the same pattern:

        name = inputname;
        if (!SolrIdentifierValidator.validate(name))
          throw IllegalArgumentException("exactly the same error message");
        

        so it seemed to make sense to me to pull that into the method. The boolean method is still in SolrIdentifierValidator, although it's now private, but I can make it public again if that helps things.

        re changing the Exception type, I agree that it's not ideal, but keeping it as IAE ended up losing information when exceptions were propagated in TestCollectionAPI. Looking at it again, though, this is mainly due to the tests there not using CollectionsAdminRequests explicitly, so there's no client-side checking. I'll see if I can change it back again.

        Show
        romseygeek Alan Woodward added a comment - Everywhere that was actually using the validator had exactly the same pattern: name = inputname; if (!SolrIdentifierValidator.validate(name)) throw IllegalArgumentException( "exactly the same error message" ); so it seemed to make sense to me to pull that into the method. The boolean method is still in SolrIdentifierValidator, although it's now private, but I can make it public again if that helps things. re changing the Exception type, I agree that it's not ideal, but keeping it as IAE ended up losing information when exceptions were propagated in TestCollectionAPI. Looking at it again, though, this is mainly due to the tests there not using CollectionsAdminRequests explicitly, so there's no client-side checking. I'll see if I can change it back again.
        Hide
        anshumg Anshum Gupta added a comment -

        I am sure that the validator response was used to throw either the SolrException of the IAE depending upon where it was. I am still not convinced with that particular change of switching the return type and calling the single validate method. Let us keep that as it was.

        Also, returning boolean allows us to change the logging, or the behavior in case of failed validation. We could still do that by catching the exception, but why even return something on success, the original code returned nothing from the validation method (void) and seems like your change partially reverts back to the old code.

        Show
        anshumg Anshum Gupta added a comment - I am sure that the validator response was used to throw either the SolrException of the IAE depending upon where it was. I am still not convinced with that particular change of switching the return type and calling the single validate method. Let us keep that as it was. Also, returning boolean allows us to change the logging, or the behavior in case of failed validation. We could still do that by catching the exception, but why even return something on success, the original code returned nothing from the validation method (void) and seems like your change partially reverts back to the old code.
        Hide
        anshumg Anshum Gupta added a comment -

        This seems to have also broken split shard. I am assuming you relied on the ref guide, which says that the shard name is mandatory for SPLITSHARD, but that's not true when split.key param is used. I'll fix this and also fix the documentation.

        Show
        anshumg Anshum Gupta added a comment - This seems to have also broken split shard. I am assuming you relied on the ref guide, which says that the shard name is mandatory for SPLITSHARD, but that's not true when split.key param is used. I'll fix this and also fix the documentation.
        Hide
        anshumg Anshum Gupta added a comment -

        Reopening to fix split shard and other open issues.

        Show
        anshumg Anshum Gupta added a comment - Reopening to fix split shard and other open issues.
        Hide
        anshumg Anshum Gupta added a comment -

        Patch that applies over SOLR-8790 so that this could be tested.

        Show
        anshumg Anshum Gupta added a comment - Patch that applies over SOLR-8790 so that this could be tested.
        Hide
        anshumg Anshum Gupta added a comment -

        SOLR-8790 has been committed. The previous patch had an issue that's been fixed in this one. The constructor for SplitShard request didn't set the value of collection name.
        Running tests. I'll commit this right after they pass.

        Show
        anshumg Anshum Gupta added a comment - SOLR-8790 has been committed. The previous patch had an issue that's been fixed in this one. The constructor for SplitShard request didn't set the value of collection name. Running tests. I'll commit this right after they pass.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-8765: Fix CollectionAdminRequest.SplitShard to accept requests without the 'shard' parameter

        Show
        jira-bot ASF subversion and git services added a comment - Commit b0caca3b60b8653a5b2539c39455bf06bcc407bf in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b0caca3 ] SOLR-8765 : Fix CollectionAdminRequest.SplitShard to accept requests without the 'shard' parameter
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit bc41158648fc031cc1fe7b07ed62f5f140854f59 in lucene-solr's branch refs/heads/branch_6x from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc41158 ]

        SOLR-8765: Fix CollectionAdminRequest.SplitShard to accept requests without the 'shard' parameter

        Show
        jira-bot ASF subversion and git services added a comment - Commit bc41158648fc031cc1fe7b07ed62f5f140854f59 in lucene-solr's branch refs/heads/branch_6x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc41158 ] SOLR-8765 : Fix CollectionAdminRequest.SplitShard to accept requests without the 'shard' parameter
        Hide
        anshumg Anshum Gupta added a comment -

        Alan Woodward when do you plan on reverting the SolrIdentifierValidator.validate usage and behavior to not throw an exception directly? If you don't have the time, I can take care of it.

        Show
        anshumg Anshum Gupta added a comment - Alan Woodward when do you plan on reverting the SolrIdentifierValidator.validate usage and behavior to not throw an exception directly? If you don't have the time, I can take care of it.
        Hide
        romseygeek Alan Woodward added a comment -

        I'll try and get to it tomorrow (UK time)

        Show
        romseygeek Alan Woodward added a comment - I'll try and get to it tomorrow (UK time)
        Hide
        dsmiley David Smiley added a comment -

        I think this issue introduced a possible problem, very likely unintended as it's easy to overlook. This is a new convenience method we are to use (or a like-kind constructor):

          public static Create createCollection(String collection, String config, int numShards, int numReplicas) {
        

        Notice that numShards is a primitive int. And notice that Create.numShards is an object Integer. the setNumShards method is deprecated so I'll overlook that as I'm not to call it. So how am I supposed to use this for the implicit router in which my intent is to manage the shards myself, without setting numShards? Perhaps we shall have a separate convenience method & constructor expressly for the implicit router?

        Show
        dsmiley David Smiley added a comment - I think this issue introduced a possible problem, very likely unintended as it's easy to overlook. This is a new convenience method we are to use (or a like-kind constructor): public static Create createCollection( String collection, String config, int numShards, int numReplicas) { Notice that numShards is a primitive int. And notice that Create.numShards is an object Integer. the setNumShards method is deprecated so I'll overlook that as I'm not to call it. So how am I supposed to use this for the implicit router in which my intent is to manage the shards myself, without setting numShards? Perhaps we shall have a separate convenience method & constructor expressly for the implicit router?
        Hide
        anshumg Anshum Gupta added a comment -

        numShards shouldn't be mandatory and so the constructor should have excluded that. I think we need tests for all of this else we wouldn't know what's even broken.
        Ideally, something that randomizes the getting of CollectionsAdminRequest objects through the deprecated constructor and the new approach would be good, but that could be a lot of effort to manage a deprecated API, so guess we should just add more tests for the new API.

        Show
        anshumg Anshum Gupta added a comment - numShards shouldn't be mandatory and so the constructor should have excluded that. I think we need tests for all of this else we wouldn't know what's even broken. Ideally, something that randomizes the getting of CollectionsAdminRequest objects through the deprecated constructor and the new approach would be good, but that could be a lot of effort to manage a deprecated API, so guess we should just add more tests for the new API.
        Hide
        romseygeek Alan Woodward added a comment -

        I'd say add another factory method for the implicit router case. The point of adding these methods is to make it a lot clearer in the API how you should use these parameters, and what combinations are allowed and which aren't.

        On tests, my plan was to gradually cut over the existing tests that use the deprecated API to the new methods, fixing stuff as I go. That's why this is targeted at 6.1, to give time to work out the wrinkles.

        Show
        romseygeek Alan Woodward added a comment - I'd say add another factory method for the implicit router case. The point of adding these methods is to make it a lot clearer in the API how you should use these parameters, and what combinations are allowed and which aren't. On tests, my plan was to gradually cut over the existing tests that use the deprecated API to the new methods, fixing stuff as I go. That's why this is targeted at 6.1, to give time to work out the wrinkles.
        Hide
        dsmiley David Smiley added a comment -

        add another factory method for the implicit router case

        +1

        Show
        dsmiley David Smiley added a comment - add another factory method for the implicit router case +1
        Hide
        anshumg Anshum Gupta added a comment -

        Do you want me to fix this?

        Show
        anshumg Anshum Gupta added a comment - Do you want me to fix this?
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7be7e8beb965714dd1fb1b85f711e9c8a882d088 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter (Unused)
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7be7e8b ]

        CHANGES.txt corrections - new features go in the New Features section (SOLR-8782, SOLR-8765, SOLR-8842)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7be7e8beb965714dd1fb1b85f711e9c8a882d088 in lucene-solr's branch refs/heads/branch_6x from Chris Hostetter (Unused) [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7be7e8b ] CHANGES.txt corrections - new features go in the New Features section ( SOLR-8782 , SOLR-8765 , SOLR-8842 )
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 423ec098504836ccd9b6e742a5b93c7b40cb0aa3 in lucene-solr's branch refs/heads/master from Chris Hostetter (Unused)
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=423ec09 ]

        CHANGES.txt corrections - new features go in the New Features section (SOLR-8782, SOLR-8765, SOLR-8842)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 423ec098504836ccd9b6e742a5b93c7b40cb0aa3 in lucene-solr's branch refs/heads/master from Chris Hostetter (Unused) [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=423ec09 ] CHANGES.txt corrections - new features go in the New Features section ( SOLR-8782 , SOLR-8765 , SOLR-8842 )
        Hide
        anshumg Anshum Gupta added a comment -

        Alan Woodward here's a patch that reverts the behavior. I plan to commit this soon but I just wanted to be sure that I'm not missing something that you see.

        Show
        anshumg Anshum Gupta added a comment - Alan Woodward here's a patch that reverts the behavior. I plan to commit this soon but I just wanted to be sure that I'm not missing something that you see.
        Hide
        romseygeek Alan Woodward added a comment -

        Hey Anshum, sorry for not getting to this sooner. Patch looks fine.

        Show
        romseygeek Alan Woodward added a comment - Hey Anshum, sorry for not getting to this sooner. Patch looks fine.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3bb4b0629825187a1ee11d2a691edddaa5d5d4a4 in lucene-solr's branch refs/heads/branch_6_0 from Varun Thacker
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3bb4b06 ]

        SchemaManager waits correctly for replicas to be notified of a new change

        For branch_6_0: Modified TestManagedSchemaAPI.java to use old-style CollectionAdminRequest.Reload, since CollectionAdminRequest.reloadCollection() from SOLR-8765 will land in 6.1.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3bb4b0629825187a1ee11d2a691edddaa5d5d4a4 in lucene-solr's branch refs/heads/branch_6_0 from Varun Thacker [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3bb4b06 ] SchemaManager waits correctly for replicas to be notified of a new change For branch_6_0: Modified TestManagedSchemaAPI.java to use old-style CollectionAdminRequest.Reload, since CollectionAdminRequest.reloadCollection() from SOLR-8765 will land in 6.1.
        Hide
        ctargett Cassandra Targett added a comment -

        Alan Woodward Where are we with this issue? It seems to be finished from the comments, but there has been enough back & forth that I'm not sure.

        Show
        ctargett Cassandra Targett added a comment - Alan Woodward Where are we with this issue? It seems to be finished from the comments, but there has been enough back & forth that I'm not sure.
        Hide
        romseygeek Alan Woodward added a comment -

        There are still gaps in the API, but I'm fixing them as I go in SOLR-9132, so I'll mark this as resolved.

        Show
        romseygeek Alan Woodward added a comment - There are still gaps in the API, but I'm fixing them as I go in SOLR-9132 , so I'll mark this as resolved.

          People

          • Assignee:
            romseygeek Alan Woodward
            Reporter:
            romseygeek Alan Woodward
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development