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

Add a "findRecursive" method to NamedList

Details

    • New Feature
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • 4.0
    • 4.4, 6.0
    • None
    • 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);
      		}
      	}
      

      Attachments

        1. SOLR-4048.patch
          5 kB
          Shawn Heisey
        2. SOLR-4048.patch
          4 kB
          Shawn Heisey
        3. SOLR-4048.patch
          4 kB
          Shawn Heisey
        4. SOLR-4048.patch
          3 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-cleanup.patch
          13 kB
          Shawn Heisey

        Activity

          elyograg 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.

          elyograg 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.
          elyograg Shawn Heisey added a comment -

          Patch with new method and a unit test.

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

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

          elyograg Shawn Heisey added a comment - New patch. The method that I copied over from my own code was synchronized. I forgot to remove that.
          elyograg Shawn Heisey added a comment -

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

          elyograg Shawn Heisey added a comment - This patch is against branch_4x. I have not checked to see whether it applies as-is against trunk.
          elyograg Shawn Heisey added a comment -

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

          elyograg Shawn Heisey added a comment - New patch using String... instead of String[].
          elyograg 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.

          elyograg 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.
          elyograg Shawn Heisey added a comment -

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

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

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

          elyograg Shawn Heisey added a comment - Previous patch was susceptible to ClassCastExceptions when sent invalid information. Fixed and updated tests.
          elyograg 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.

          elyograg 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.
          rcmuir 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.

          rcmuir 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.
          elyograg Shawn Heisey added a comment -

          Updated patch following advice from rcmuir and improving the javadoc.

          elyograg Shawn Heisey added a comment - Updated patch following advice from rcmuir and improving the javadoc.
          elyograg Shawn Heisey added a comment -

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

          elyograg Shawn Heisey added a comment - Adding find and deprecating get seems reasonable to me, though it would be a new issue.
          elyograg 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.

          elyograg 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.
          elyograg Shawn Heisey added a comment -

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

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

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

          elyograg Shawn Heisey added a comment - Committed to trunk (r1484135) and branch_4x (r1484137).
          elyograg 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.

          jpountz 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.

          elyograg 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. jpountz 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.
          jpountz 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>.

          jpountz 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>.
          elyograg Shawn Heisey added a comment -

          thetaphi 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.

          elyograg Shawn Heisey added a comment - thetaphi 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.
          uschindler 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.

          uschindler 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.
          elyograg 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?

          elyograg 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?
          elyograg 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.

          elyograg 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.
          uschindler Uwe Schindler added a comment -

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

          uschindler Uwe Schindler added a comment - Looks much better! The return type seems to be correct. The instanceof check was also fixed to be effective.
          uschindler 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

          uschindler 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
          elyograg 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.

          elyograg 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.
          uschindler Uwe Schindler added a comment -

          All fine now!

          uschindler Uwe Schindler added a comment - All fine now!
          elyograg 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.

          elyograg 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.
          sarowe Steven Rowe added a comment -

          Bulk close resolved 4.4 issues

          sarowe Steven Rowe added a comment - Bulk close resolved 4.4 issues

          People

            elyograg Shawn Heisey
            elyograg Shawn Heisey
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: