Fix Issue #997: Preserve ConfigEntry order during merge operations

[ravisastryk]

The merge() method was reversing entry order by inserting at position 0. Fixed by collecting entries first and using addAll(0, collection) which preserves iteration order.

Fixes #997 - ConfigEntry order is now preserved during merge operations.

Background and Context

When merging configurations, entries were inserted at position 0 individually. This was causing the final order to be reversed from the source file order.

# Config file
KEY = valueA
KEY = valueB
KEY = valueC

• Direct load: [valueA, valueB, valueC]
• After merge: [valueC, valueB, valueA]

Why This Works

List.addAll(int index, Collection c) inserts all elements in iteration order starting at the specified index. The existing elements shift right to accommodate.

Before: serviceParameters = [X, Y, Z]
addAll(0, [A, B, C])
After:  serviceParameters = [A, B, C, X, Y, Z]  <----- Order is preserved

Why This Is Better

Aspect	add(0, x) in loop	addAll(0, collection)
Correctness	Reverses order	Preserves order
Performance	O(n²) - shifts array n times	O(n) - single shift operation
Atomicity	Multiple mutations	Single list operation

Fix

Replace the loop of add(0, entry) calls with a single addAll(0, entries) which preserves iteration order while still placing merged entries at the front.

Changes

• ServiceConfigGuide.merge(): Use addAll(0, ...) instead of individual insertions
• Added regression tests for entry ordering

Tests

• All existing tests pass
• New tests verify order preservation on merge

[drivenflywheel]

Code and tests look good. Will have to evaluate downstream impacts prior to downstream integration in case any existing configs deliberately leverage the existing behavior. Those downstream configs could be reworked, but they'll need to be identified.

[rcameronm]

Directly modifying the behavior of the merge method like this seems potentially dangerous because of possible downstream effects. I think a safer alternative is to create a new overloaded handleNewEntry with an additional parameter that determines whether or not to prepend or append the merged configs. The default behavior would remain prepending so we don't break anything, and an additional overloaded merge method can then take the prepend/append arg.

[ravisastryk]

Code and tests look good. Will have to evaluate downstream impacts prior to downstream integration in case any existing configs deliberately leverage the existing behavior. Those downstream configs could be reworked, but they'll need to be identified.

That's a good point. However, if downstream systems are affected, they were relying on buggy behavior and need fixing anyway and we never documented this behavior and it is actually fixing the original bug #997

[ravisastryk]

Thank you @rupert-griffin @drivenflywheel for reviewing my PR. When you get a chance, please re-review. I updated test to make it clear. Thank you for your time and consideration.

[ravisastryk]

@rupert-griffin @drivenflywheel Could you please review again? Thank you

[ravisastryk]

@rupert-griffin @jpdahlke @drivenflywheel Can you please re-review?

[ravisastryk]

@dev-mlb @sambish5 Can you please review when you get a chance?

[drivenflywheel]

@ravisastryk - I understand that you're eager to get this merged, but it's not likely to happen in the near term. This PR changes heavily-leveraged behavior that's been in place for years - and in a good way - but once this is merged downstream integrators won't be able to accept any other emissary changes unless they also ensure that this change won't impact their runtime behavior. That can be a heavy lift, and we don't want this change to impede any urgent needs that may come up.

[ravisastryk]

@ravisastryk - I understand that you're eager to get this merged, but it's not likely to happen in the near term. This PR changes heavily-leveraged behavior that's been in place for years - and in a good way - but once this is merged downstream integrators won't be able to accept any other emissary changes unless they also ensure that this change won't impact their runtime behavior. That can be a heavy lift, and we don't want this change to impede any urgent needs that may come up.

Thank you for the thoughtful explanation @drivenflywheel I appreciate the transparency about the downstream concerns and the care taken to protect integrators from unexpected disruptions. That said, I'd love to keep this conversation going so we can find a path forward when the timing is right. A few thoughts that might help frame that discussion:

1. Backward-compatible bridging: @rupert-griffin's suggestion of overloading handleNewEntry and merge with a prepend/append parameter is a great middle ground — it keeps existing behavior as the default while making the correct behavior opt-in. Happy to implement that if it would lower the integration risk.

2. Feature flag / config knob: If it helps, the behavior change could be gated behind a config flag so integrators can adopt it on their own timeline.

No pressure on timing. I just want to make sure the fix for #997 stays on the radar and that there's a clear path to eventually landing it. Happy to take direction on whichever approach the team prefers.