Skip to content

Commit 7e6e503

Browse files
ZachBrayZachBray
authored
[Java, C#, C++] Fix precedence checks to account for non-contiguous versions. (#989)
Relates to issue #988. An SBE message might not change in every version of a schema. For example, another message might change. Previously, the field precedence checking model would expect an exact match for `actingVersion` in `wrap` to enter one of its initial states. However, it is only aware of the versions in which a message schema has changed. Therefore, it was possible for `actingVersion` not to match any of these versions, in which case the initial state that represented a codec wrapped around the latest known version of the format was picked. Now, we do _not_ expect an exact match in `wrap`. Instead, we select the "best" match. For example, if a codec was changed in v1 and v3, and the acting version is v2, we will decode using v1. However, if the acting version is v4, we would decode as v3.
1 parent c9be629 commit 7e6e503

File tree

10 files changed

+257
-63
lines changed

10 files changed

+257
-63
lines changed

‎csharp/sbe-tests/FieldAccessOrderCheckTests.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3159,6 +3159,25 @@ public void DisallowsIncompleteMessagesDueToMissingVarDataInNestedGroup(
31593159
var exception = Assert.ThrowsException<InvalidOperationException>(encoder.CheckEncodingIsComplete);
31603160
StringAssert.Contains(exception.Message, $"Not fully encoded, current state: {expectedState}");
31613161
}
3162+
3163+
[TestMethod]
3164+
public void AllowsSkippingFutureGroupWhenDecodingFromVersionWithNoChangesInMessage()
3165+
{
3166+
var encoder = new AddGroupBeforeVarDataV0()
3167+
.WrapForEncodeAndApplyHeader(_buffer, Offset, _messageHeader);
3168+
3169+
_messageHeader.TemplateId = SkipVersionAddGroupBeforeVarDataV2.TemplateId;
3170+
_messageHeader.Version = 1;
3171+
3172+
encoder.A = 42;
3173+
encoder.SetB("abc");
3174+
3175+
var decoder = new SkipVersionAddGroupBeforeVarDataV2()
3176+
.WrapForDecodeAndApplyHeader(_buffer, Offset, _messageHeader);
3177+
3178+
Assert.AreEqual(42, decoder.A);
3179+
Assert.AreEqual(decoder.GetB(), "abc");
3180+
}
31623181

31633182
private void ModifyHeaderToLookLikeVersion0()
31643183
{

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/common/FieldPrecedenceModel.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public final class FieldPrecedenceModel
4646
new CodecInteraction.CodecInteractionFactory(groupPathsByField, topLevelBlockFields);
4747
private final Map<CodecInteraction, List<TransitionGroup>> transitionsByInteraction = new LinkedHashMap<>();
4848
private final Map<State, List<TransitionGroup>> transitionsByState = new HashMap<>();
49-
private final Int2ObjectHashMap<State> versionWrappedStates = new Int2ObjectHashMap<>();
49+
private final TreeMap<Integer, State> versionWrappedStates = new TreeMap<>();
5050
private final State notWrappedState = allocateState("NOT_WRAPPED");
5151
private final String generatedRepresentationClassName;
5252
private State encoderWrappedState;
@@ -103,13 +103,11 @@ public State latestVersionWrappedState()
103103
* Iterates over the states after a codec is wrapped over a particular version of data.
104104
* @param consumer the consumer of the states.
105105
*/
106-
public void forEachWrappedStateByVersion(final IntObjConsumer<State> consumer)
106+
public void forEachWrappedStateByVersionDesc(final IntObjConsumer<State> consumer)
107107
{
108-
final Int2ObjectHashMap<State>.EntryIterator iterator = versionWrappedStates.entrySet().iterator();
109-
while (iterator.hasNext())
108+
for (final Map.Entry<Integer, State> entry : versionWrappedStates.descendingMap().entrySet())
110109
{
111-
iterator.next();
112-
consumer.accept(iterator.getIntKey(), iterator.getValue());
110+
consumer.accept(entry.getKey(), entry.getValue());
113111
}
114112
}
115113

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/cpp/CppGenerator.java

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3055,22 +3055,37 @@ private static CharSequence generateDecoderWrapListener(final FieldPrecedenceMod
30553055

30563056
final StringBuilder sb = new StringBuilder();
30573057
sb.append(INDENT).append("void onWrapForDecode(std::uint64_t actingVersion)\n")
3058-
.append(INDENT).append("{\n")
3059-
.append(INDENT).append(INDENT).append("switch(actingVersion)\n")
3060-
.append(INDENT).append(INDENT).append("{\n");
3061-
3062-
fieldPrecedenceModel.forEachWrappedStateByVersion((version, state) ->
3063-
sb.append(INDENT).append(TWO_INDENT).append("case ").append(version).append(":\n")
3064-
.append(INDENT).append(THREE_INDENT).append("codecState(")
3065-
.append(qualifiedStateCase(state)).append(");\n")
3066-
.append(INDENT).append(THREE_INDENT).append("break;\n"));
3067-
3068-
sb.append(INDENT).append(TWO_INDENT).append("default:\n")
3069-
.append(INDENT).append(THREE_INDENT).append("codecState(")
3070-
.append(qualifiedStateCase(fieldPrecedenceModel.latestVersionWrappedState())).append(");\n")
3071-
.append(INDENT).append(THREE_INDENT).append("break;\n")
3072-
.append(INDENT).append(INDENT).append("}\n")
3073-
.append(INDENT).append("}\n\n");
3058+
.append(INDENT).append("{\n");
3059+
3060+
final MutableBoolean actingVersionCanBeTooLowToBeValid = new MutableBoolean(true);
3061+
3062+
fieldPrecedenceModel.forEachWrappedStateByVersionDesc((version, state) ->
3063+
{
3064+
if (version == 0)
3065+
{
3066+
actingVersionCanBeTooLowToBeValid.set(false);
3067+
3068+
sb.append(INDENT).append(" codecState(")
3069+
.append(qualifiedStateCase(state)).append(");\n");
3070+
}
3071+
else
3072+
{
3073+
sb.append(INDENT).append(" if (actingVersion >= ").append(version).append(")\n")
3074+
.append(INDENT).append(" {\n")
3075+
.append(INDENT).append(" codecState(")
3076+
.append(qualifiedStateCase(state)).append(");\n")
3077+
.append(INDENT).append(" return;\n")
3078+
.append(INDENT).append(" }\n\n");
3079+
}
3080+
});
3081+
3082+
if (actingVersionCanBeTooLowToBeValid.get())
3083+
{
3084+
sb.append(INDENT)
3085+
.append(" throw std::runtime_error(\"Unsupported acting version: \" + actingVersion);\n");
3086+
}
3087+
3088+
sb.append(INDENT).append("}\n\n");
30743089

30753090
return sb;
30763091
}

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/csharp/CSharpGenerator.java

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1912,22 +1912,20 @@ private static CharSequence generateDecoderWrapListener(
19121912
}
19131913

19141914
final StringBuilder sb = new StringBuilder();
1915+
19151916
sb.append(indent).append("private void OnWrapForDecode(int actingVersion)\n")
1916-
.append(indent).append("{\n")
1917-
.append(indent).append(INDENT).append("switch(actingVersion)\n")
1918-
.append(indent).append(INDENT).append("{\n");
1917+
.append(indent).append("{\n");
19191918

1920-
fieldPrecedenceModel.forEachWrappedStateByVersion((version, state) ->
1921-
sb.append(indent).append(TWO_INDENT).append("case ").append(version).append(":\n")
1922-
.append(indent).append(THREE_INDENT).append("codecState(")
1919+
fieldPrecedenceModel.forEachWrappedStateByVersionDesc((version, state) ->
1920+
sb.append(indent).append(" if (actingVersion >= ").append(version).append(")\n")
1921+
.append(indent).append(" {\n")
1922+
.append(indent).append(" codecState(")
19231923
.append(qualifiedStateCase(state)).append(");\n")
1924-
.append(indent).append(THREE_INDENT).append("break;\n"));
1924+
.append(indent).append(" return;\n")
1925+
.append(indent).append(" }\n\n"));
19251926

1926-
sb.append(indent).append(TWO_INDENT).append("default:\n")
1927-
.append(indent).append(THREE_INDENT).append("codecState(")
1928-
.append(qualifiedStateCase(fieldPrecedenceModel.latestVersionWrappedState())).append(");\n")
1929-
.append(indent).append(THREE_INDENT).append("break;\n")
1930-
.append(indent).append(INDENT).append("}\n")
1927+
sb.append(indent)
1928+
.append(" throw new InvalidOperationException(\"Unsupported acting version: \" + actingVersion);\n")
19311929
.append(indent).append("}\n\n");
19321930

19331931
return sb;

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaGenerator.java

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -762,21 +762,18 @@ private static CharSequence generateDecoderWrapListener(
762762

763763
final StringBuilder sb = new StringBuilder();
764764
sb.append(indent).append("private void onWrap(final int actingVersion)\n")
765-
.append(indent).append("{\n")
766-
.append(indent).append(" switch(actingVersion)\n")
767-
.append(indent).append(" {\n");
765+
.append(indent).append("{\n");
768766

769-
fieldPrecedenceModel.forEachWrappedStateByVersion((version, state) ->
770-
sb.append(indent).append(" case ").append(version).append(":\n")
771-
.append(indent).append(" codecState(")
767+
fieldPrecedenceModel.forEachWrappedStateByVersionDesc((version, state) ->
768+
sb.append(indent).append(" if (actingVersion >= ").append(version).append(")\n")
769+
.append(indent).append(" {\n")
770+
.append(indent).append(" codecState(")
772771
.append(qualifiedStateCase(state)).append(");\n")
773-
.append(indent).append(" break;\n"));
772+
.append(indent).append(" return;\n")
773+
.append(indent).append(" }\n\n"));
774774

775-
sb.append(indent).append(" default:\n")
776-
.append(indent).append(" codecState(")
777-
.append(qualifiedStateCase(fieldPrecedenceModel.latestVersionWrappedState())).append(");\n")
778-
.append(indent).append(" break;\n")
779-
.append(indent).append(" }\n")
775+
sb.append(indent)
776+
.append(" throw new IllegalStateException(\"Unsupported acting version: \" + actingVersion);\n")
780777
.append(indent).append("}\n\n");
781778

782779
return sb;

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/generated/FrameCodecDecoder.java

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,15 +139,13 @@ public int offset()
139139

140140
private void onWrap(final int actingVersion)
141141
{
142-
switch(actingVersion)
142+
if (actingVersion >= 0)
143143
{
144-
case 0:
145-
codecState(CodecStates.V0_BLOCK);
146-
break;
147-
default:
148-
codecState(CodecStates.V0_BLOCK);
149-
break;
144+
codecState(CodecStates.V0_BLOCK);
145+
return;
150146
}
147+
148+
throw new IllegalStateException("Unsupported acting version: " + actingVersion);
151149
}
152150

153151
public FrameCodecDecoder wrap(

‎sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/generated/TokenCodecDecoder.java

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -186,15 +186,13 @@ public int offset()
186186

187187
private void onWrap(final int actingVersion)
188188
{
189-
switch(actingVersion)
189+
if (actingVersion >= 0)
190190
{
191-
case 0:
192-
codecState(CodecStates.V0_BLOCK);
193-
break;
194-
default:
195-
codecState(CodecStates.V0_BLOCK);
196-
break;
191+
codecState(CodecStates.V0_BLOCK);
192+
return;
197193
}
194+
195+
throw new IllegalStateException("Unsupported acting version: " + actingVersion);
198196
}
199197

200198
public TokenCodecDecoder wrap(

‎sbe-tool/src/test/cpp/FieldAccessOrderCheckTest.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
#include "order_check/NoBlock.h"
5252
#include "order_check/GroupWithNoBlock.h"
5353
#include "order_check/NestedGroupWithVarLength.h"
54+
#include "order_check/SkipVersionAddGroupBeforeVarDataV2.h"
5455

5556
using namespace order::check;
5657
using ::testing::HasSubstr;
@@ -4935,3 +4936,22 @@ INSTANTIATE_TEST_SUITE_P(
49354936
std::make_tuple(2, 2, "V0_B_N_D_N_BLOCK")
49364937
)
49374938
);
4939+
4940+
TEST_F(FieldAccessOrderCheckTest, allowsSkippingFutureGroupWhenDecodingFromVersionWithNoChangesInMessagePart1)
4941+
{
4942+
AddGroupBeforeVarDataV0 encoder;
4943+
encoder.wrapForEncode(m_buffer, OFFSET, BUFFER_LEN);
4944+
encoder.a(42).putB("abc");
4945+
4946+
SkipVersionAddGroupBeforeVarDataV2 decoder;
4947+
decoder.wrapForDecode(
4948+
m_buffer,
4949+
OFFSET,
4950+
AddGroupBeforeVarDataV0::sbeBlockLength(),
4951+
1,
4952+
BUFFER_LEN
4953+
);
4954+
4955+
EXPECT_EQ(decoder.a(), 42);
4956+
EXPECT_EQ(decoder.getBAsString(), "abc");
4957+
}

‎sbe-tool/src/test/java/uk/co/real_logic/sbe/FieldAccessOrderCheckTest.java

Lines changed: 113 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.junit.jupiter.api.Test;
2727
import org.junit.jupiter.params.ParameterizedTest;
2828
import org.junit.jupiter.params.provider.CsvSource;
29+
import org.junit.jupiter.params.provider.ValueSource;
2930

3031
import java.nio.charset.StandardCharsets;
3132
import java.util.Random;
@@ -3407,6 +3408,116 @@ void disallowsIncompleteMessagesDueToMissingVarDataInNestedGroup(
34073408
assertThat(exception.getMessage(), containsString("Not fully encoded, current state: " + expectedState));
34083409
}
34093410

3411+
@Test
3412+
void decodesNullValueForFutureVersionBlockFieldWhenCurrentVersionHasNoChangesInMessage()
3413+
{
3414+
final MessageHeaderEncoder headerEncoder = new MessageHeaderEncoder();
3415+
final SkipVersionAddPrimitiveV1Encoder encoder = new SkipVersionAddPrimitiveV1Encoder()
3416+
.wrapAndApplyHeader(buffer, OFFSET, headerEncoder);
3417+
3418+
headerEncoder
3419+
.templateId(SkipVersionAddPrimitiveV2Encoder.TEMPLATE_ID)
3420+
.version(1);
3421+
3422+
encoder.b("abc");
3423+
3424+
final SkipVersionAddPrimitiveV2Decoder decoder = new SkipVersionAddPrimitiveV2Decoder()
3425+
.wrapAndApplyHeader(buffer, OFFSET, messageHeaderDecoder);
3426+
3427+
assertThat(decoder.a(), equalTo(SkipVersionAddPrimitiveV2Decoder.aNullValue()));
3428+
assertThat(decoder.b(), equalTo("abc"));
3429+
}
3430+
3431+
@Test
3432+
void decodesEmptyFutureGroupWhenDecodingFromVersionWithNoChangesInMessage()
3433+
{
3434+
final MessageHeaderEncoder headerEncoder = new MessageHeaderEncoder();
3435+
final SkipVersionAddGroupV1Encoder encoder = new SkipVersionAddGroupV1Encoder()
3436+
.wrapAndApplyHeader(buffer, OFFSET, headerEncoder);
3437+
3438+
headerEncoder
3439+
.templateId(SkipVersionAddGroupV2Decoder.TEMPLATE_ID)
3440+
.version(1);
3441+
3442+
encoder.a(42);
3443+
3444+
final SkipVersionAddGroupV2Decoder decoder = new SkipVersionAddGroupV2Decoder()
3445+
.wrapAndApplyHeader(buffer, OFFSET, messageHeaderDecoder);
3446+
3447+
assertThat(decoder.a(), equalTo(42));
3448+
assertThat(decoder.b().count(), equalTo(0));
3449+
}
3450+
3451+
@Test
3452+
void decodesNullValueForFutureVarDataFieldWhenDecodingFromVersionWithNoChangesInMessage()
3453+
{
3454+
final MessageHeaderEncoder headerEncoder = new MessageHeaderEncoder();
3455+
final SkipVersionAddVarDataV1Encoder encoder = new SkipVersionAddVarDataV1Encoder()
3456+
.wrapAndApplyHeader(buffer, OFFSET, headerEncoder);
3457+
3458+
headerEncoder
3459+
.templateId(SkipVersionAddVarDataV2Decoder.TEMPLATE_ID)
3460+
.version(1);
3461+
3462+
encoder.a(42);
3463+
3464+
final SkipVersionAddVarDataV2Decoder decoder = new SkipVersionAddVarDataV2Decoder()
3465+
.wrapAndApplyHeader(buffer, OFFSET, messageHeaderDecoder);
3466+
3467+
assertThat(decoder.a(), equalTo(42));
3468+
assertThat(decoder.b(), equalTo(""));
3469+
}
3470+
3471+
@Test
3472+
void allowsDecodingUnknownFutureVersions()
3473+
{
3474+
final MessageHeaderEncoder headerEncoder = new MessageHeaderEncoder();
3475+
final SkipVersionAddPrimitiveV2Encoder encoder = new SkipVersionAddPrimitiveV2Encoder()
3476+
.wrapAndApplyHeader(buffer, OFFSET, headerEncoder);
3477+
3478+
headerEncoder
3479+
.templateId(SkipVersionAddPrimitiveV2Encoder.TEMPLATE_ID)
3480+
.version(42);
3481+
3482+
encoder.a(43).b("abc");
3483+
3484+
final SkipVersionAddPrimitiveV2Decoder decoder = new SkipVersionAddPrimitiveV2Decoder()
3485+
.wrapAndApplyHeader(buffer, OFFSET, messageHeaderDecoder);
3486+
3487+
assertThat(decoder.a(), equalTo(43));
3488+
assertThat(decoder.b(), equalTo("abc"));
3489+
}
3490+
3491+
@ParameterizedTest
3492+
@ValueSource(booleans = {false, true})
3493+
void allowsSkippingFutureGroupWhenDecodingFromVersionWithNoChangesInMessage(final boolean decodeGroup)
3494+
{
3495+
final MessageHeaderEncoder headerEncoder = new MessageHeaderEncoder();
3496+
final AddGroupBeforeVarDataV0Encoder encoder = new AddGroupBeforeVarDataV0Encoder()
3497+
.wrapAndApplyHeader(buffer, OFFSET, headerEncoder);
3498+
3499+
headerEncoder
3500+
.templateId(SkipVersionAddGroupBeforeVarDataV2Decoder.TEMPLATE_ID)
3501+
.version(1);
3502+
3503+
encoder.a(42).b("abc");
3504+
3505+
final SkipVersionAddGroupBeforeVarDataV2Decoder decoder = new SkipVersionAddGroupBeforeVarDataV2Decoder()
3506+
.wrapAndApplyHeader(buffer, OFFSET, messageHeaderDecoder);
3507+
3508+
assertThat(decoder.a(), equalTo(42));
3509+
3510+
if (decodeGroup)
3511+
{
3512+
for (final SkipVersionAddGroupBeforeVarDataV2Decoder.CDecoder group : decoder.c())
3513+
{
3514+
group.sbeSkip();
3515+
}
3516+
}
3517+
3518+
assertThat(decoder.b(), equalTo("abc"));
3519+
}
3520+
34103521
private void modifyHeaderToLookLikeVersion0()
34113522
{
34123523
messageHeaderDecoder.wrap(buffer, OFFSET);
@@ -3418,9 +3529,9 @@ private void modifyHeaderToLookLikeVersion0()
34183529
private void modifyHeaderToLookLikeVersion1()
34193530
{
34203531
messageHeaderDecoder.wrap(buffer, OFFSET);
3421-
assert messageHeaderDecoder.version() == 1;
3532+
assert messageHeaderDecoder.version() >= 1;
34223533
final int v0TemplateId = messageHeaderDecoder.templateId() - 1_000;
34233534
messageHeaderEncoder.wrap(buffer, OFFSET);
3424-
messageHeaderEncoder.templateId(v0TemplateId);
3535+
messageHeaderEncoder.templateId(v0TemplateId).version(1);
34253536
}
34263537
}

0 commit comments

Comments
 (0)