Preserve order of @JsonAnySetter creator property arguments

[eranl]

Make @JsonAnySetter creator properties behave similarly to non-creator ones, using a LinkedHashMap.
Since the arguments are stored in a reverse-order singly-linked list, I used recursion to restore the original order. Any better way to do it?

Fixes #5353.

[cowtowncoder]

I think we'd first need an issue explaining what is the problem to fix.

[cowtowncoder]

Ok, use of LinkedHashMap in itself is acceptable, retaining order possibly useful, with modest (probably non-measurable) overhead.

But adding overhead of reordering for every use is not good -- many users would get penalized despite not caring about ordering.

So if we can figure out how to insert values in order, +1.

@JooHyukKim knows this area pretty well and can probably help.

[eranl]

But adding overhead of reordering for every use is not good -- many users would get penalized despite not caring about ordering.

You mean the recursion? Is that measurable?

[cowtowncoder]

But adding overhead of reordering for every use is not good -- many users would get penalized despite not caring about ordering.

You mean the recursion? Is that measurable?

No I mean reordering in general; should be possible to just retain insert order without essentially ordering twice.

[eranl]

The only change I made there was replacing iteration with recursion.

[cowtowncoder]

The only change I made there was replacing iteration with recursion.

Hmh. Ok, I probably need to re-read PR.

But yes, recursion on its own can also be problematic wrt security aspects (stack overflow) so ideally not usad with linear data structures.

[cowtowncoder]

Ok yes, recursion to re-order, since it's effectively linked list. Hmmh.
I'll have to think about, no obvious better way to go about that.

[JooHyukKim]

Sorry late reply.

1. Iteration does sound safer than recursion, since anySetter pretty much as no limit to size.
2. Another question is... would it be possible fixing the way data is passed to record the record? @cowtowncoder Simply reversing back sounds okay, but seems hacky instead of fixing the actual problem/or issue
3. If we do add order-preservation, need to add a new feature (unless performance is preserved)

[eranl]

I considered replacing the linked list with some List, but that seemed like a big change. Happy to do it if you guys think it's the right way to go.

[cowtowncoder]

I'm not happy with recursion due to possible stack exhaustion. But adding an extra data structure not great either.

I think we need to have this as opt-in (DeserializationFeature) for this, to explicitly enable.

[eranl]

adding an extra data structure not great either.

I'm proposing a replacement data structure, not an extra one.

[cowtowncoder]

One other note: while retaining order sounds like an improvement, it also has some backwards-compatibility aspects so I think that actual change should probably go against 3.x branch, not 2.x.
We are just now switching to focusing 2.x on particularly low-risk fixes, changes, and most new features (and riskier changes) to 3.x only.
(basically we will merge forward from 2.x to 3.x quite easily but not the reverse).

So, if -- for example, PropertyValueBuffer buffering was changed from reverse-linked (or, prepend-only) to append-linked (so "head" and "tail" pointers) to retain ordering for all buffered properties -- this would seem potentially risky for some usage: while users should NOT rely on ordering of deserialization during buffering, it's almost guaranteed someone somewhere is relying on it (likely unintentionally/unknowingly).

But +1 for improved data structure, fix against 3.x.

[eranl]

I guess that makes sense, but it requires that I upgrade to v3 first, so it'll take a while...

[cowtowncoder]

I guess that makes sense, but it requires that I upgrade to v3 first, so it'll take a while...

Understood.

[cowtowncoder]

I think change would need to be for PropertyValue and PropertyValueBuffer, to change single link to head (currently) to "head" (first entry, starting as null) and "tail" (last one added). For PropertyValue, "next" would need to be mutable as direction changed. Doable, just a fair bit of work....

[JooHyukKim]

I think change would need to be for PropertyValue and PropertyValueBuffer, to change single link to head (currently) to "head" (first entry, starting as null) and "tail" (last one added). For PropertyValue, "next" would need to be mutable as direction changed. Doable, just a fair bit of work....

@eranl are u still able to pick up where you left off?

[eranl]

I will be able to at some point, but not soon.

[cowtowncoder]

Alt. implementation via #5766, closing this pr.