Solr
  1. Solr
  2. SOLR-8360

ExternalFileField.getValueSource uses req.datadir but this.schema

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      ExternalFileField.getValueSource(SchemaField field, QParser parser) has available:

      • datadir
        • parser.getReq().getCore().getDataDir()
        • this.schema.getResourceLoader().getDataDir()
      • schema
        • parser.getReq().getSchema()
        • this.schema

      ExternalFileField.getValueSource uses parser.getReq().getCore().getDataDir() explicitly but implicitly this.schema - should it use parser.getReq().getSchema() instead (Option 1 patch)? Or if in practice actually req.datadir and this.datadir are always the same could we stop using the parser argument (Option 2 patch (1 line))?

      1. SOLR-8360-option1.patch
        2 kB
        Christine Poerschke
      2. SOLR-8360-option2.patch
        0.6 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          Christine Poerschke added a comment -

          alternative patches against trunk

          Show
          Christine Poerschke added a comment - alternative patches against trunk
          Hide
          Christine Poerschke added a comment -

          Hi Alan Woodward - saw your name in the history of this class/method and wondered if you would have any thoughts on this JIRA here? As I see it, if req.datadir and this.datadir are always the same (and req.schema and this.schema are always the same) then the 'Option 2' patch would simplify the code. On the other hand, if req.schema and this.schema can sometimes be different then the 'Option 1' patch might be fixing an edge case bug? Or 'Option 3' could be to only add a clarifying comment that and why this.schema rather than req.schema is used combined with req.datadir.

          Show
          Christine Poerschke added a comment - Hi Alan Woodward - saw your name in the history of this class/method and wondered if you would have any thoughts on this JIRA here? As I see it, if req.datadir and this.datadir are always the same (and req.schema and this.schema are always the same) then the 'Option 2' patch would simplify the code. On the other hand, if req.schema and this.schema can sometimes be different then the 'Option 1' patch might be fixing an edge case bug? Or 'Option 3' could be to only add a clarifying comment that and why this.schema rather than req.schema is used combined with req.datadir.
          Hide
          Alan Woodward added a comment -

          The data directory should always be the same, so I think your second patch is the way to go.

          Show
          Alan Woodward added a comment - The data directory should always be the same, so I think your second patch is the way to go.
          Hide
          ASF subversion and git services added a comment -

          Commit 1718562 from Christine Poerschke in branch 'dev/trunk'
          [ https://svn.apache.org/r1718562 ]

          SOLR-8360: simplify ExternalFileField.getValueSource implementation

          Show
          ASF subversion and git services added a comment - Commit 1718562 from Christine Poerschke in branch 'dev/trunk' [ https://svn.apache.org/r1718562 ] SOLR-8360 : simplify ExternalFileField.getValueSource implementation
          Hide
          ASF subversion and git services added a comment -

          Commit 1718580 from Christine Poerschke in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1718580 ]

          SOLR-8360: simplify ExternalFileField.getValueSource implementation (merge in revision 1718562 from trunk)

          Show
          ASF subversion and git services added a comment - Commit 1718580 from Christine Poerschke in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1718580 ] SOLR-8360 : simplify ExternalFileField.getValueSource implementation (merge in revision 1718562 from trunk)

            People

            • Assignee:
              Christine Poerschke
              Reporter:
              Christine Poerschke
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development