[
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:
----------------------------------
[[email protected]], 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.
[[email protected]], 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: [email protected]
For additional commands, e-mail: [email protected]
