Solr
  1. Solr
  2. SOLR-4048

Add a "findRecursive" method to NamedList

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0
    • Fix Version/s: 4.4, 5.0
    • Component/s: None
    • Labels:
      None

      Description

      Most of the time when accessing data from a NamedList, what you'll be doing is using get() to retrieve another NamedList, and doing so over and over until you reach the final level, where you'll actually retrieve the value you want.

      I propose adding a method to NamedList which would do all that heavy lifting for you. I created the following method for my own code. It could be adapted fairly easily for inclusion into NamedList itself. The only reason I did not include it as a patch is because I figure you'll want to ensure it meets all your particular coding guidelines, and that the JavaDoc is much better than I have done here:

      	/**
      	 * Recursively parse a NamedList and return the value at the last level,
      	 * assuming that the object found at each level is also a NamedList. For
      	 * example, if "response" is the NamedList response from the Solr4 mbean
      	 * handler, the following code makes sense:
      	 * 
      	 * String coreName = (String) getRecursiveFromResponse(response, new
      	 * String[] { "solr-mbeans", "CORE", "core", "stats", "coreName" });
      	 * 
      	 * 
      	 * @param namedList the NamedList to parse
      	 * @param args A list of values to recursively request
      	 * @return the object at the last level.
      	 * @throws SolrServerException
      	 */
      	@SuppressWarnings("unchecked")
      	private final Object getRecursiveFromResponse(
      			NamedList<Object> namedList, String[] args)
      			throws SolrServerException
      	{
      
      		NamedList<Object> list = null;
      		Object value = null;
      		try
      		{
      			for (String key : args)
      			{
      				if (list == null)
      				{
      					list = namedList;
      				}
      				else
      				{
      					list = (NamedList<Object>) value;
      				}
      				value = list.get(key);
      			}
      			return value;
      		}
      		catch (Exception e)
      		{
      			throw new SolrServerException(
      					"Failed to recursively parse NamedList", e);
      		}
      	}
      
      1. SOLR-4048-cleanup.patch
        13 kB
        Shawn Heisey
      2. SOLR-4048.patch
        5 kB
        Shawn Heisey
      3. SOLR-4048.patch
        4 kB
        Shawn Heisey
      4. SOLR-4048.patch
        4 kB
        Shawn Heisey
      5. SOLR-4048.patch
        3 kB
        Shawn Heisey
      6. SOLR-4048.patch
        3 kB
        Shawn Heisey
      7. SOLR-4048.patch
        3 kB
        Shawn Heisey
      8. SOLR-4048.patch
        3 kB
        Shawn Heisey

        Activity

        Steve Rowe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues
        Shawn Heisey made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 5.0 [ 12321664 ]
        Resolution Fixed [ 1 ]
        Hide
        Shawn Heisey added a comment -

        On 4x, two solr-core tests failed with 500 server errors during shard splitting. I've seen this over and over in Jenkins, so I'm pretty sure it's not my changes. All solr-solrj tests pass fine, and precommit passes. The branch_4x commit is r1484548.

        Show
        Shawn Heisey added a comment - On 4x, two solr-core tests failed with 500 server errors during shard splitting. I've seen this over and over in Jenkins, so I'm pretty sure it's not my changes. All solr-solrj tests pass fine, and precommit passes. The branch_4x commit is r1484548.
        Hide
        Uwe Schindler added a comment -

        All fine now!

        Show
        Uwe Schindler added a comment - All fine now!
        Hide
        Shawn Heisey added a comment -

        Trunk commit r1484386 to improve and clean up new method. All tests and precommit passed. Will run tests and precommit on 4x before backporting tomorrow.

        Show
        Shawn Heisey added a comment - Trunk commit r1484386 to improve and clean up new method. All tests and precommit passed. Will run tests and precommit on 4x before backporting tomorrow.
        Hide
        Uwe Schindler added a comment - - edited

        The cleanup patch also fixes all warnings at the 1.7 compliance level in trunk.

        Warnings are disabled in Solr's ANT build because whole Solr is a huge warning

        Show
        Uwe Schindler added a comment - - edited The cleanup patch also fixes all warnings at the 1.7 compliance level in trunk. Warnings are disabled in Solr's ANT build because whole Solr is a huge warning
        Hide
        Uwe Schindler added a comment -

        Looks much better! The return type seems to be correct. The instanceof check was also fixed to be effective.

        Show
        Uwe Schindler added a comment - Looks much better! The return type seems to be correct. The instanceof check was also fixed to be effective.
        Hide
        Shawn Heisey added a comment - - edited

        The cleanup patch also fixes all warnings at the 1.7 compliance level in trunk. I don't think the ant build showed any warnings, but they were showing up in Eclipse.

        Show
        Shawn Heisey added a comment - - edited The cleanup patch also fixes all warnings at the 1.7 compliance level in trunk. I don't think the ant build showed any warnings, but they were showing up in Eclipse.
        Shawn Heisey made changes -
        Attachment SOLR-4048-cleanup.patch [ 12583780 ]
        Hide
        Shawn Heisey added a comment -

        On advice from Adrien and Uwe, I have refactored the new method to use an Object return type and <?> instead of generics. I tried to create a version that uses position data like get(String,int), but that didn't work out as expected, so I gave up on it.

        Attaching a cleanup patch against current trunk. This time everything is fine in 4x as well. Could I get a fine-tooth-comb review before committing this time?

        Show
        Shawn Heisey added a comment - On advice from Adrien and Uwe, I have refactored the new method to use an Object return type and <?> instead of generics. I tried to create a version that uses position data like get(String,int), but that didn't work out as expected, so I gave up on it. Attaching a cleanup patch against current trunk. This time everything is fine in 4x as well. Could I get a fine-tooth-comb review before committing this time?
        Hide
        Uwe Schindler added a comment - - edited

        Just merge it after you committed to trunk, no need to revert. I just wanted to fix the compile failure. You can revert it locally in your checkout and then merge it with your trunk commit and finally commit both changes in one op.

        The test with Integer and int is somehow not really useful, because it does not test NamedList, it just tests autoboxing and the java compiler. I would reduce it to one integer test and use (Integer) as cast.

        Show
        Uwe Schindler added a comment - - edited Just merge it after you committed to trunk, no need to revert. I just wanted to fix the compile failure. You can revert it locally in your checkout and then merge it with your trunk commit and finally commit both changes in one op. The test with Integer and int is somehow not really useful, because it does not test NamedList, it just tests autoboxing and the java compiler. I would reduce it to one integer test and use (Integer) as cast.
        Hide
        Shawn Heisey added a comment -

        Uwe Schindler I appreciate your assist on the 4x build failure from my commit. I'd like to commit a slightly different fix in trunk and have the same code in 4x, but I don't feel comfortable with the idea of reverting your commit, especially without your permission. Could you do that? I'll be watching, I'm ready with my trunk commit.

        Show
        Shawn Heisey added a comment - Uwe Schindler I appreciate your assist on the 4x build failure from my commit. I'd like to commit a slightly different fix in trunk and have the same code in 4x, but I don't feel comfortable with the idea of reverting your commit, especially without your permission. Could you do that? I'll be watching, I'm ready with my trunk commit.
        Hide
        Adrien Grand added a comment -

        Indeed I think the return type of the method is wrong since the return type of the method cannot be known at compile time. For example a NamedList<NamedList<Integer>> would return NamedList<NamedList<Integer>> if the array length is 0, NamedList<Integer> if the array length is 1 and Integer if the array length is 2. There is a similar problem for intermediate NamedLists which are all casted to a NamedList<T> although it's not their type. So the return type should probably be Object and the intermediate NamedLists should be casted to NamedList<?> instead of NamedList<T>.

        Show
        Adrien Grand added a comment - Indeed I think the return type of the method is wrong since the return type of the method cannot be known at compile time. For example a NamedList<NamedList<Integer>> would return NamedList<NamedList<Integer>> if the array length is 0, NamedList<Integer> if the array length is 1 and Integer if the array length is 2. There is a similar problem for intermediate NamedLists which are all casted to a NamedList<T> although it's not their type. So the return type should probably be Object and the intermediate NamedLists should be casted to NamedList<?> instead of NamedList<T>.
        Shawn Heisey made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Shawn Heisey added a comment -

        Test errors on 4x - 1.6 compliance doesn't like what I did with int/Integer, so jenkins blew up.

        Adrien Grand has questions about whether it should use Object and <?> instead of generics. This is an area where my understanding isn't enough to know the right path.

        Show
        Shawn Heisey added a comment - Test errors on 4x - 1.6 compliance doesn't like what I did with int/Integer, so jenkins blew up. Adrien Grand has questions about whether it should use Object and <?> instead of generics. This is an area where my understanding isn't enough to know the right path.
        Shawn Heisey made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Shawn Heisey [ elyograg ]
        Resolution Fixed [ 1 ]
        Hide
        Shawn Heisey added a comment -

        Committed to trunk (r1484135) and branch_4x (r1484137).

        Show
        Shawn Heisey added a comment - Committed to trunk (r1484135) and branch_4x (r1484137).
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12583689 ]
        Hide
        Shawn Heisey added a comment -

        new patch. CHANGES.txt and a little more verbose testing.

        Show
        Shawn Heisey added a comment - new patch. CHANGES.txt and a little more verbose testing.
        Hide
        Shawn Heisey added a comment -

        I'm going to add at least one more test - to validate what happens with some different object types.

        After updating the patch for that, I'm planning to commit this in the next few hours unless there's a reasonable objection. Even if I did something wrong in the new method, the patch won't break anything. Due to the test additions, I'm reasonably confident that the coding is correct.

        Show
        Shawn Heisey added a comment - I'm going to add at least one more test - to validate what happens with some different object types. After updating the patch for that, I'm planning to commit this in the next few hours unless there's a reasonable objection. Even if I did something wrong in the new method, the patch won't break anything. Due to the test additions, I'm reasonably confident that the coding is correct.
        Shawn Heisey made changes -
        Summary Add a "getRecursive" method to NamedList Add a "findRecursive" method to NamedList
        Hide
        Shawn Heisey added a comment -

        Adding find and deprecating get seems reasonable to me, though it would be a new issue.

        Show
        Shawn Heisey added a comment - Adding find and deprecating get seems reasonable to me, though it would be a new issue.
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12583359 ]
        Hide
        Shawn Heisey added a comment -

        Updated patch following advice from Robert Muir and improving the javadoc.

        Show
        Shawn Heisey added a comment - Updated patch following advice from Robert Muir and improving the javadoc.
        Hide
        Robert Muir added a comment -

        Maybe name the method 'findRecursive' due to its runtime?

        I really wish the existing get() was named 'find' for the same reason.

        Show
        Robert Muir added a comment - Maybe name the method 'findRecursive' due to its runtime? I really wish the existing get() was named 'find' for the same reason.
        Hide
        Shawn Heisey added a comment -

        If there is no objection, I will clean this patch up and commit. I'll put it in for 4.4, not 4.3.

        Show
        Shawn Heisey added a comment - If there is no objection, I will clean this patch up and commit. I'll put it in for 4.4, not 4.3.
        Shawn Heisey made changes -
        Fix Version/s 4.4 [ 12324324 ]
        Fix Version/s 4.3 [ 12324128 ]
        Robert Muir made changes -
        Fix Version/s 4.3 [ 12324128 ]
        Fix Version/s 5.0 [ 12321664 ]
        Fix Version/s 4.2 [ 12323893 ]
        Mark Miller made changes -
        Fix Version/s 4.2 [ 12323893 ]
        Fix Version/s 5.0 [ 12321664 ]
        Fix Version/s 4.1 [ 12321141 ]
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12560119 ]
        Hide
        Shawn Heisey added a comment -

        Previous patch was susceptible to ClassCastExceptions when sent invalid information. Fixed and updated tests.

        Show
        Shawn Heisey added a comment - Previous patch was susceptible to ClassCastExceptions when sent invalid information. Fixed and updated tests.
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12560111 ]
        Hide
        Shawn Heisey added a comment -

        New patch with improved javadoc. Patch is against trunk, but applies cleanly to branch_4x as well.

        Show
        Shawn Heisey added a comment - New patch with improved javadoc. Patch is against trunk, but applies cleanly to branch_4x as well.
        Hide
        Shawn Heisey added a comment -

        The javadoc for this new method probably should say "One or more" instead of "An array of" for the args parameter.

        I am curious about whether this can be considered for inclusion in Solr.

        Show
        Shawn Heisey added a comment - The javadoc for this new method probably should say "One or more" instead of "An array of" for the args parameter. I am curious about whether this can be considered for inclusion in Solr.
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12553936 ]
        Hide
        Shawn Heisey added a comment -

        New patch using String... instead of String[].

        Show
        Shawn Heisey added a comment - New patch using String... instead of String[].
        Hide
        Shawn Heisey added a comment -

        This patch is against branch_4x. I have not checked to see whether it applies as-is against trunk.

        Show
        Shawn Heisey added a comment - This patch is against branch_4x. I have not checked to see whether it applies as-is against trunk.
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12553085 ]
        Hide
        Shawn Heisey added a comment -

        New patch. The method that I copied over from my own code was synchronized. I forgot to remove that.

        Show
        Shawn Heisey added a comment - New patch. The method that I copied over from my own code was synchronized. I forgot to remove that.
        Shawn Heisey made changes -
        Attachment SOLR-4048.patch [ 12553083 ]
        Hide
        Shawn Heisey added a comment -

        Patch with new method and a unit test.

        Show
        Shawn Heisey added a comment - Patch with new method and a unit test.
        Hide
        Shawn Heisey added a comment -

        Had to edit that. I have my own Exception type, I forgot to change one of the lines to SolrServerException.

        Show
        Shawn Heisey added a comment - Had to edit that. I have my own Exception type, I forgot to change one of the lines to SolrServerException.
        Shawn Heisey made changes -
        Field Original Value New Value
        Description Most of the time when accessing data from a NamedList, what you'll be doing is using get() to retrieve another NamedList, and doing so over and over until you reach the final level, where you'll actually retrieve the value you want.

        I propose adding a method to NamedList which would do all that heavy lifting for you. I created the following method for my own code. It could be adapted fairly easily for inclusion into NamedList itself. The only reason I did not include it as a patch is because I figure you'll want to ensure it meets all your particular coding guidelines, and that the JavaDoc is much better than I have done here:

        {code}
        /**
        * Recursively parse a NamedList and return the value at the last level,
        * assuming that the object found at each level is also a NamedList. For
        * example, if "response" is the NamedList response from the Solr4 mbean
        * handler, the following code makes sense:
        *
        * String coreName = (String) getRecursiveFromResponse(response, new
        * String[] { "solr-mbeans", "CORE", "core", "stats", "coreName" });
        *
        *
        * @param namedList the NamedList to parse
        * @param args A list of values to recursively request
        * @return the object at the last level.
        * @throws SolrServerException
        */
        @SuppressWarnings("unchecked")
        private final Object getRecursiveFromResponse(
        NamedList<Object> namedList, String[] args)
        throws CommonSolrException
        {

        NamedList<Object> list = null;
        Object value = null;
        try
        {
        for (String key : args)
        {
        if (list == null)
        {
        list = namedList;
        }
        else
        {
        list = (NamedList<Object>) value;
        }
        value = list.get(key);
        }
        return value;
        }
        catch (Exception e)
        {
        throw new SolrServerException(
        "Failed to recursively parse NamedList", e);
        }
        }
        {code}
        Most of the time when accessing data from a NamedList, what you'll be doing is using get() to retrieve another NamedList, and doing so over and over until you reach the final level, where you'll actually retrieve the value you want.

        I propose adding a method to NamedList which would do all that heavy lifting for you. I created the following method for my own code. It could be adapted fairly easily for inclusion into NamedList itself. The only reason I did not include it as a patch is because I figure you'll want to ensure it meets all your particular coding guidelines, and that the JavaDoc is much better than I have done here:

        {code}
        /**
        * Recursively parse a NamedList and return the value at the last level,
        * assuming that the object found at each level is also a NamedList. For
        * example, if "response" is the NamedList response from the Solr4 mbean
        * handler, the following code makes sense:
        *
        * String coreName = (String) getRecursiveFromResponse(response, new
        * String[] { "solr-mbeans", "CORE", "core", "stats", "coreName" });
        *
        *
        * @param namedList the NamedList to parse
        * @param args A list of values to recursively request
        * @return the object at the last level.
        * @throws SolrServerException
        */
        @SuppressWarnings("unchecked")
        private final Object getRecursiveFromResponse(
        NamedList<Object> namedList, String[] args)
        throws SolrServerException
        {

        NamedList<Object> list = null;
        Object value = null;
        try
        {
        for (String key : args)
        {
        if (list == null)
        {
        list = namedList;
        }
        else
        {
        list = (NamedList<Object>) value;
        }
        value = list.get(key);
        }
        return value;
        }
        catch (Exception e)
        {
        throw new SolrServerException(
        "Failed to recursively parse NamedList", e);
        }
        }
        {code}
        Shawn Heisey created issue -

          People

          • Assignee:
            Shawn Heisey
            Reporter:
            Shawn Heisey
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development