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

Add convenience method to ModifiableSolrParams

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.6
    • Component/s: SolrJ
    • Labels:
      None
    • Flags:
      Patch

      Description

      Add a static convenience method ModifiableSolrParams#of(SolrParams) which returns the same instance if it already is modifiable, otherwise creates a new ModifiableSolrParams instance.

      Rationale: when writing custom SearchComponents, we find that we often need to ensure that the SolrParams are modifiable. The copy constructor of ModifiableSolrParams always creates a copy, even if the SolrParms already are modifiable.

      Alternatives: The method could also be added as a convenience method in SolrParams itself, which already has static helper methods for wrapDefaults and wrapAppended.

      1. SOLR-9184.patch
        3 kB
        Jörg Rathlev
      2. SOLR-9184.patch
        3 kB
        Jörg Rathlev

        Activity

        Hide
        elyograg Shawn Heisey added a comment - - edited

        There's no need to remove previous patches. Jira automatically handles marking the old ones differently than the newest one with the same filename. Having previous versions of a patch available helps to understand the thought processes of the person who created the patches – we prefer to keep that history, even if the older patches are blatantly wrong.

        Although I like this change, it probably should be restricted to master (7.0-SNAPSHOT) because it could break existing code. There are likely people who intentionally use the constructor to copy one MSP object, creating a new one that can be modified independently of the original. If we change that behavior in a minor release, any user who depends on that behavior will have a broken program after upgrading SolrJ. There might even be instances in the Solr main or test code that rely on this behavior – all uses must be reviewed.

        Edit: Closer reading reveals that you want to add a method. Which makes my concern moot.

        Show
        elyograg Shawn Heisey added a comment - - edited There's no need to remove previous patches. Jira automatically handles marking the old ones differently than the newest one with the same filename. Having previous versions of a patch available helps to understand the thought processes of the person who created the patches – we prefer to keep that history, even if the older patches are blatantly wrong. Although I like this change, it probably should be restricted to master (7.0-SNAPSHOT) because it could break existing code. There are likely people who intentionally use the constructor to copy one MSP object, creating a new one that can be modified independently of the original. If we change that behavior in a minor release, any user who depends on that behavior will have a broken program after upgrading SolrJ. There might even be instances in the Solr main or test code that rely on this behavior – all uses must be reviewed. Edit: Closer reading reveals that you want to add a method. Which makes my concern moot.
        Hide
        joergr Jörg Rathlev added a comment -

        Sorry about removing the first patch file. I accidentally left a println added for debugging in the first version and removed it.

        This change simply adds a static factory method, it does not propose to modify the existing constructor. I agree that intentional use of the copy constructor is a valid use case that must be supported.

        Show
        joergr Jörg Rathlev added a comment - Sorry about removing the first patch file. I accidentally left a println added for debugging in the first version and removed it. This change simply adds a static factory method, it does not propose to modify the existing constructor. I agree that intentional use of the copy constructor is a valid use case that must be supported.
        Hide
        dsmiley David Smiley added a comment -

        +1 to this. Solr should use this internally at any applicable spot where it's doing the instanceof check today.

        Show
        dsmiley David Smiley added a comment - +1 to this. Solr should use this internally at any applicable spot where it's doing the instanceof check today.
        Hide
        koji Koji Sekiguchi added a comment -

        I think this is almost ready. How about adding assertNotSame for the first test?

        Show
        koji Koji Sekiguchi added a comment - I think this is almost ready. How about adding assertNotSame for the first test?
        Hide
        joergr Jörg Rathlev added a comment -

        I agree, that's a good idea. I also noticed that the null-input case was not tested yet, so I've added that as well.

        Updated patch attached.

        Show
        joergr Jörg Rathlev added a comment - I agree, that's a good idea. I also noticed that the null-input case was not tested yet, so I've added that as well. Updated patch attached.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-9184: Add a static convenience method ModifiableSolrParams#of(SolrParams) which returns the same instance if it already is modifiable, otherwise creates a new ModifiableSolrParams instance.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 583fec1a58b41a0562529e6228a29728a790d87c in lucene-solr's branch refs/heads/master from koji [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=583fec1 ] SOLR-9184 : Add a static convenience method ModifiableSolrParams#of(SolrParams) which returns the same instance if it already is modifiable, otherwise creates a new ModifiableSolrParams instance.
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        SOLR-9184: Add a static convenience method ModifiableSolrParams#of(SolrParams) which returns the same instance if it already is modifiable, otherwise creates a new ModifiableSolrParams instance.

        Show
        jira-bot ASF subversion and git services added a comment - Commit bcd452c6a1026bcec814e355b8366c9b4ece49e0 in lucene-solr's branch refs/heads/branch_6x from koji [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bcd452c ] SOLR-9184 : Add a static convenience method ModifiableSolrParams#of(SolrParams) which returns the same instance if it already is modifiable, otherwise creates a new ModifiableSolrParams instance.
        Hide
        koji Koji Sekiguchi added a comment -

        Thanks, Jörg!

        Show
        koji Koji Sekiguchi added a comment - Thanks, Jörg!
        Hide
        dsmiley David Smiley added a comment -

        Koji Sekiguchi Welcome back to committing to Solr

        Show
        dsmiley David Smiley added a comment - Koji Sekiguchi Welcome back to committing to Solr

          People

          • Assignee:
            koji Koji Sekiguchi
            Reporter:
            joergr Jörg Rathlev
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development