Thanks for the thorough review Steve, very helpful! The updated patch cleans up a number of the issues you raised, specifically:
1. In SolrDispatchFilter.doFilter(): The section starting if( path.startsWith("/rest") ) appears to be a vestige?
Fixed (was vestige). Currently, the RestManager supports /schema and /config only.
2. There's no way to create a new resource (i.e., one not mentioned in schema.xml/solrconfig.xml)
The RestManager's ManagedEndpoint class is now attached as the default resource class (no longer DefaultSchemaResource). If a request to create a new resource comes in, the PUT/POST request will be routed to the RestManager's RestManagerManagedResource, which knows how to create new managed resources and make them active in the server.
3. I don't see any REST API tests?
Most of the API testing is in the concrete endpoints (stop words / synonym) as most of what needs to be tested in the RestManager layer can be tested without making REST API calls. However, I've also added some additional REST API tests to TestRestManager.
4. I think we should limit the permissible top-level registerable paths, currently to /schema/ and /config/.
Done, but required the introduction of SolrConfigRestApi class and changes to Solr's web.xml. At this point, ManagedResource implementers have to choose which path makes the most sense for their resource, see ManagedStopFilterFactory as an example.
We may be able to get away with 1 class that extends Restlet's Application by implementing a custom Finder. However, I wanted to get something in place that solidifies the paths the API will support as soon as possible. If we determine a cleaner way to support /schema and /config, then we should be able to do that without breaking client code.
5. In looking at the patch on
SOLR-5641, which will need to be changed to use the RestManager solution developed here ...
TBD - That's a large patch that will need more time to go over. There is some overlap in that this patch also includes a SolrConfigRestApi.
6. It's important that configuration in schema.xml/solrconfig.xml remains valid regardless of the operation of the RestManager interacting with managed resources
I've added a check for having additional args to the BaseManagedTokenFilterFactory however there's nothing in the RestManager framework that enforces this.
The main idea behind my implementation is that the "managed" attribute is the only one declared and then all initArgs are stored/managed in the managed data. That said, I could see the validity of supporting invariant initArgs so this might need some more work.
7. About your known issue c): "Deletes - the ManagedResource framework supports deletes but I wasn't sure how to enable them in Restlet" - not sure about Restlet, but there are two Solr code locations I know of that need to be changed to enable new HTTP verbs (I ran into this when I added PUT support): SolrRequestParsers.parseParamsAndFillStreams() and SolrDispatchFilter.remoteQuery().
Thanks. Deletes are working now. However, a ManagedResource cannot be deleted if there are Solr components still using it.
8. In ManagedWordSetResource, onManagedDataLoadedFromStorage() and updateInitArgs(), the String.toLowerCase() calls should be given a Locale argument (likely Locale.ROOT) to escape the wrath of forbidden-apis. Running ant precommit at the top level will catch this kind of problem (and other things).
9. You didn't mention it here, but if we want to enable using PATCH REST calls via Restlet, we'll need to upgrade Restlet from the current v2.1.1 to at least v2.2M1 - note that v2.2RC1 was just released. (See the issue adding PATCH support and the v2.2RC1 release announcement.)
We should tackle upgrading Restlet in a different JIRA issue and then PATCH support can be added later if desired.
10. In ManagedWordSetResource.updateInitArgs(), if ignoreCase is changed from true to false, the previous downcasing operation can't be undone. Not sure the best way to handle this: maybe serialization should always capture the unprocessed original versions?
This is a tough one. My thinking is that we shouldn't worry about this for now since it seems like more work / trouble supporting than it may be worth and seems like a client app issue vs. something that needs to be built-in to the RestManager framework. That said, I'm also open to hearing about cases where we should support this.
11. ManagedWordSetResource.doGet() ignores ignoreCase
Fixed. Improved the unit tests to verify this as well.
12. In the current model, all managed resource GET calls will return dirty managed resource data, rather than live data. I think it's important to make the dirty data accessible, but we should consider whether live data should be accessible in addition, maybe as a param to the GET call?
The use case I designed for was that updates would be applied using a core / collection reload very soon after submitting changes to the API. In other words, the time where live data and dirty data are different should very small and not all that important. Mainly, I don't want to start going down the path of implementing as version control system for what is supposed to be a simple API for changing config settings and then reloading the core to apply them.
13. In your tests, you escape double-quotes in JSON strings, but there's a nice method in SolrTestCaseJ4 named json() that will accept single-quoted string values and auto-convert to double-quotes. Makes the JSON much easier to look at without all the backslashes.
Fixed. Thanks for tip, much nicer to look at indeed.
14. ManagedResource assumes JSON storage format, but the idea is to override methods to handle other formats, right? I think there should be a complete example of doing that in a test.
Added. Please see - TestManagedResource#testCustomStorageFormat
15. It seems weird to me that PUT requests in your patch respond with the same information as GET would against the same resource. Maybe just return a string indicating success? (I might be off-base here, I haven't looked at many examples of this elsewhere.)
Agreed on the weird part and fixed. Now just returns 200 status code
16. The patch ignores the (wt param) on GET methods - this should be fairly simple to fix, by storing data structures rather than JSON strings on the response; see e.g. CopyFieldCollectionResource.get().
Hmmm... Not sure what's up with this one as using wt=xml works for me without changes. To be sure, I added test to verify XML as well as JSON.
17. In RestManager.doInit() on line 128, you have a TODO:
Good catch ... Thanks!