[ https://issues.apache.org/jira/browse/SOLR-6315?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14084077#comment-14084077 ]
Shai Erera commented on SOLR-6315: ---------------------------------- [~yo...@apache.org], please excuse me if I used the word "bogus" inappropriately. I'll admit that as a non-native English speaker, I often make mistakes. I assumed that the word "bogus" is just a synonym for "fake", but perhaps it's used in the English language in negative contexts. If this is the case, this was not my intention ... you know me better than that. But I do think this class pretends to be something that it is not. Its documentation is misleading and conflicting ("use if it access by key is more important..." and "this class does not provide efficient lookup by key"). The class itself contains absolutely no code beyond NamedList. It contains 3 ctors which delegate to super() as-is, and a clone() which is exactly like NamedList's, only returns a SimpleOrderedMap. Also, the class's name is misleading -- see Jack's comment above and the word 'order'. It looks as if the class pretends to be an ordered map, where its jdocs specifically say that you should use it if you prefer access by key than maintaining order... I was merely pointing facts about the class, and why I think it should be removed. Maybe once it had another role, or actually did something different than NamedList, but it isn't today. Having said all that, you don't see me commit anything, or jump to conclusions .. I sincerely ask for guidance from people who know this code better than I do, like you, what's the best way to deal with it. [~markrmil...@gmail.com], I don't think that leaving this class alone is a good resolution in this case. I consider redundant code as something that needs to go. Especially if it's confusing, and more so public API. Even if not a user-facing API, it's still an API w/ many uses in the code. I realize there are probably other places I can contribute to Solr, much more important ones, but this is where I started when I reviewed the code. I'm BTW not trying to fix this class, but to fix/simplify NamedList overall (see my last comment on SOLR-912). This helps me get familiar with the code, and I'm sure that I'll learn more as I get comments about what I'm trying to do. Sometimes a fresh set of eyes lead to simplifications that others just don't have the time to notice. More so, I feel that JSONResponseWriter misbehaves. I'm sure that there are good reasons to it, too. But the way I see it, no matter what style you ask it to output, if it encounters a SimpleOrderedMap, it always output it as a "map". This feels wrong to me ... And worse, if I'm a Solr developer and need to create a NamedList instance, I need to choose between Named to Simple because of how they are output differently. And, the tests that currently fail, only fail because they assume some undocumented behavior of JSONResponseWriter (maybe as Yonik said, it's because this isn't well tested). Why would a test assume that the JSON response it parsed, is parsed to a Map, when the default style is "flat"? And of course, as soon as I removed the instanceof check in JSONResponseWriter, these tests failed to cast a List to a Map. This leads me to believe that the effective default style is "map", no matter what the code says. As a side comment I stated that I find it odd that a JSON response writer/formatter's default style is not common JSON. Not that an array output is not valid, just that it's not common. And I admit that I don't know the reason for the "flat" style, but I'm sure if it's there, it's probably used by someone. Look at JSONWriterTest.toJson(). It creates a SolrQueryResponse (which uses SimpleOrderedMap internally) and a JSONResponseWriter with style "arrarr" (array-of-array). Yet it expects an output that looks like a "map". In fact, it's a mixed style of "map" and "flat"/"arr". This is because JSONResponseWriter completely ignores the style once it encounters the SimpleOrderedMap. I don't understand if this is really an expected behavior, or this test tests buggy behavior... why would such a test ever pass, when the output is not what it expects? So again, this started on my part as a simple attempt to improve and cleanup NamedList and friends. But I now wonder if our JSON formatter does the right thing or not. > Remove SimpleOrderedMap > ----------------------- > > Key: SOLR-6315 > URL: https://issues.apache.org/jira/browse/SOLR-6315 > Project: Solr > Issue Type: Improvement > Components: clients - java > Reporter: Shai Erera > Assignee: Shai Erera > Attachments: SOLR-6315.patch > > > As I described on SOLR-912, SimpleOrderedMap is redundant and generally > useless class, with confusing jdocs. We should remove it. I'll attach a patch > shortly. -- This message was sent by Atlassian JIRA (v6.2#6252) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org