On Fri, Nov 16, 2012 at 4:57 PM, Yonik Seeley <[email protected]> wrote:
> On Fri, Nov 16, 2012 at 11:56 AM, Eirik Lygre <[email protected]> > wrote: > > One way out would be to have a parallel set of methods that work on > symbolic > > form: These new functions would not be able to parse a sort string, but > they > > would handle more complex sort specifications. As a user of SolrQuery, > you > > would use one, but not both, of these apis. > > That seems reasonable to me. Something like this perhaps? > > Map<String, SORT_ORDER> getSort() > setSort(Map<String, SORT_ORDER>) > String getSortString() > void setSortString(String sortString) // throw exception if the > internal Map isn't null? > > and add/removeSortField would only work on the internal Map (and throw > an exception if setSortString had been used?) > A couple of thoughts: 1) The old methods work on the serialized string. If we want to maintain backward compatibility (do we?), we can't really change that, because that supports a use case where you first set the full sort string, and then start manipulating them. 2) I'm not sure about the map-idea, but not all against it either :-) a) If we expose the Map as the api, we should probably specify that this is an ordered map; otherwise, people will make the common mistake of using a HashMap, inserting values in the order they want fields sorted, and get the unexpected result. As far as I know, that means specifying LinkedHashMap, right? b) We also need to decide on the point of serialization, and the mutability of the map returned from getSort(). - If it is mutable, I expect to be able to say "query.getSort().put("field", ORDER.asc). For that to work, we need to have a clear serialization entry point, and I haven't found that. - If it is immutable, there will be some amount of copying maps which really isn't needed, perhaps c) I'm not sure about the value of the exception from setSortString; set(CommonParams.SORT, str) would presumably bypass that anyway, unless we override that, too. And, if we are to serialize the string as the map changes (which I think we might have to), we'll probably need some local (but simple) bookkeeping in order to keep track of the source of the sort string (symbolic vs raw api). If we were to maintain backward compatibility, I would think of something like this: // Functional, symbolic api. Changes are serialized immediately Map<String, ORDER> getSort() // Returns immmutable, ordered map setSort(LinkedHashMap<S,O>); addSort(String, ORDER); removeSort(); removeSort(String); // Raw string api, shortcuts to get()/set() with CommonParams.SORT void setSortString(String sortString) String getSortString() // @Deprecated api, parses the serialized string setSortField(String, ORDER) addSortField(String, ORDER) removeSortField(String, ORDER) String[] getSortFields() String getSortField() // Same as getSortString() If we skip backward compatibility (e.g. no use case with setting the string and then manipulating it), we get off easier, much like your suggestion: // Raw string api, shortcuts to get()/set() with CommonParams.SORT void setSortField(String sortString) // New String getSortField() // Functional, symbolic api, changes the serialized string Map<String, ORDER> getSort() // Returns immmutable, ordered map setSort(LinkedHashMap<>) setSortField(String, ORDER) addSortField(String, ORDER) removeSortField(String, ORDER) String[] getSortFields() // Array of serialized entries So, a couple of questions out of this: - Do we want to maintain backward compatibility i.e. the use case with setting the string and then manipulating it? - Is there a clear point for serialization, so that we can serialize "at the end" only? -- Eirik There is no high like a tango high There is no low like a tango low
