Skip to content

Commit 90ad392

Browse files
JordonPhillipsJordonPhillips
committed
Use natural dict ordering for member index
This updates schemas to use the natural ordering of a dict to set member index. Recursive shapes are accommodated by using a None value to reserve the member's spot in the dictionary until the value can be actually set. This has two primary effects. Firstly, it allows the member index to be confidently used at runtime to actually retrieve a member. This isn't needed in most cases, but can come in handy occasionally. This also cuts out a huge amount of boilerplate in the generated code.
1 parent 3afcd5a commit 90ad392

File tree

10 files changed

+220
-163
lines changed

10 files changed

+220
-163
lines changed

‎codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SchemaGenerator.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,11 @@ private void writeSchemaMembers(PythonWriter writer, Shape shape) {
162162
for (MemberShape member : shape.members()) {
163163
if (!generatedShapes.contains(member.getTarget()) && !Prelude.isPreludeShape(member.getTarget())) {
164164
deferredMembers.put(member, index++);
165+
writer.write("""
166+
# This needs to reference a schema that isn't defined yet.
167+
# It will be populated with a non-null value at the end of the file.
168+
$S: None,
169+
""", member.getMemberName());
165170
continue;
166171
}
167172

@@ -183,7 +188,6 @@ private void writeSchemaMembers(PythonWriter writer, Shape shape) {
183188
writer.write("""
184189
$S: {
185190
"target": $T,
186-
"index": $L,
187191
${?hasTraits}
188192
"traits": [
189193
${C|}
@@ -193,7 +197,6 @@ private void writeSchemaMembers(PythonWriter writer, Shape shape) {
193197
""",
194198
member.getMemberName(),
195199
targetSchemaSymbol,
196-
index,
197200
writer.consumer(w -> writeTraits(w, traits)));
198201

199202
index++;

‎designs/serialization.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ EXAMPLE_STRUCTURE_SCHEMA = Schema.collection(
121121
members={
122122
"member": {
123123
"target": INTEGER,
124-
"index": 0,
125124
"traits": [
126125
DefaultTrait(0),
127126
],

‎packages/smithy-aws-event-stream/tests/unit/_private/__init__.py‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -38,32 +38,29 @@
3838
SCHEMA_MESSAGE_EVENT = Schema.collection(
3939
id=ShapeID("smithy.example#MessageEvent"),
4040
members={
41-
"boolHeader": {"index": 0, "target": BOOLEAN, "traits": [EVENT_HEADER_TRAIT]},
42-
"byteHeader": {"index": 1, "target": BYTE, "traits": [EVENT_HEADER_TRAIT]},
43-
"shortHeader": {"index": 2, "target": SHORT, "traits": [EVENT_HEADER_TRAIT]},
44-
"intHeader": {"index": 3, "target": INTEGER, "traits": [EVENT_HEADER_TRAIT]},
45-
"longHeader": {"index": 4, "target": LONG, "traits": [EVENT_HEADER_TRAIT]},
46-
"blobHeader": {"index": 5, "target": BLOB, "traits": [EVENT_HEADER_TRAIT]},
47-
"stringHeader": {"index": 6, "target": STRING, "traits": [EVENT_HEADER_TRAIT]},
41+
"boolHeader": {"target": BOOLEAN, "traits": [EVENT_HEADER_TRAIT]},
42+
"byteHeader": {"target": BYTE, "traits": [EVENT_HEADER_TRAIT]},
43+
"shortHeader": {"target": SHORT, "traits": [EVENT_HEADER_TRAIT]},
44+
"intHeader": {"target": INTEGER, "traits": [EVENT_HEADER_TRAIT]},
45+
"longHeader": {"target": LONG, "traits": [EVENT_HEADER_TRAIT]},
46+
"blobHeader": {"target": BLOB, "traits": [EVENT_HEADER_TRAIT]},
47+
"stringHeader": {"target": STRING, "traits": [EVENT_HEADER_TRAIT]},
4848
"timestampHeader": {
49-
"index": 7,
5049
"target": TIMESTAMP,
5150
"traits": [EVENT_HEADER_TRAIT],
5251
},
53-
"bodyMember": {"index": 8, "target": STRING},
52+
"bodyMember": {"target": STRING},
5453
},
5554
)
5655

5756
SCHEMA_PAYLOAD_EVENT = Schema.collection(
5857
id=ShapeID("smithy.example#PayloadEvent"),
5958
members={
6059
"header": {
61-
"index": 0,
6260
"target": STRING,
6361
"traits": [EVENT_HEADER_TRAIT, REQUIRED_TRAIT],
6462
},
6563
"payload": {
66-
"index": 1,
6764
"target": STRING,
6865
"traits": [EVENT_PAYLOAD_TRAIT, REQUIRED_TRAIT],
6966
},
@@ -74,12 +71,10 @@
7471
id=ShapeID("smithy.example#BlobPayloadEvent"),
7572
members={
7673
"header": {
77-
"index": 0,
7874
"target": STRING,
7975
"traits": [EVENT_HEADER_TRAIT, REQUIRED_TRAIT],
8076
},
8177
"payload": {
82-
"index": 1,
8378
"target": BLOB,
8479
"traits": [EVENT_PAYLOAD_TRAIT, REQUIRED_TRAIT],
8580
},
@@ -88,7 +83,7 @@
8883

8984
SCHEMA_ERROR_EVENT = Schema.collection(
9085
id=ShapeID("smithy.example#ErrorEvent"),
91-
members={"message": {"index": 0, "target": STRING, "traits": [REQUIRED_TRAIT]}},
86+
members={"message": {"target": STRING, "traits": [REQUIRED_TRAIT]}},
9287
traits=[ERROR_TRAIT],
9388
)
9489

@@ -97,21 +92,21 @@
9792
shape_type=ShapeType.UNION,
9893
traits=[STREAMING_TRAIT],
9994
members={
100-
"message": {"index": 0, "target": SCHEMA_MESSAGE_EVENT},
101-
"payload": {"index": 1, "target": SCHEMA_PAYLOAD_EVENT},
102-
"blobPayload": {"index": 2, "target": SCHEMA_BLOB_PAYLOAD_EVENT},
103-
"error": {"index": 3, "target": SCHEMA_ERROR_EVENT},
95+
"message": {"target": SCHEMA_MESSAGE_EVENT},
96+
"payload": {"target": SCHEMA_PAYLOAD_EVENT},
97+
"blobPayload": {"target": SCHEMA_BLOB_PAYLOAD_EVENT},
98+
"error": {"target": SCHEMA_ERROR_EVENT},
10499
},
105100
)
106101

107102
SCHEMA_INITIAL_MESSAGE = Schema.collection(
108103
id=ShapeID("smithy.example#EventStreamOperationInputOutput"),
109104
members={
110-
"message": {"index": 0, "target": STRING},
105+
"message": {"target": STRING},
111106
# Event stream members will not be part of the operation input / output
112107
# shape schemas.
113108
# "stream": {
114-
# "index": 1,
109+
#
115110
# "target": SCHEMA_EVENT_STREAM
116111
# },
117112
},

‎packages/smithy-core/src/smithy_core/schemas.py‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,13 @@ def __init__(
7474
traits = {}
7575
object.__setattr__(self, "traits", traits)
7676

77-
if members:
78-
if isinstance(members, list):
79-
m: dict[str, Schema] = {}
80-
for member in members:
81-
m[member.expect_member_name()] = member
82-
members = m
83-
else:
77+
if members is None:
8478
members = {}
79+
elif isinstance(members, list):
80+
m: dict[str, Schema] = {}
81+
for member in sorted(members, key=lambda m: m.expect_member_index()):
82+
m[member.expect_member_name()] = member
83+
members = m
8584
object.__setattr__(self, "members", members)
8685

8786
if member_target is not None:
@@ -194,7 +193,7 @@ def collection(
194193
id: ShapeID,
195194
shape_type: ShapeType = ShapeType.STRUCTURE,
196195
traits: list["Trait | DynamicTrait"] | None = None,
197-
members: Mapping[str, "MemberSchema"] | None = None,
196+
members: Mapping[str, "MemberSchema | None"] | None = None,
198197
) -> Self:
199198
"""Create a schema for a collection shape.
200199
@@ -205,15 +204,23 @@ def collection(
205204
properties, map keys/values, and union variants. In contrast to the main
206205
constructor, this is a dict of member names to a simplified dict containing
207206
only ``traits`` and a ``target``. Member schemas will be generated from this.
207+
208+
If the value is None, it MUST be populated later. This is to allow a preservation
209+
of modeled order without having to explicitly provide it and therefore generate
210+
a ton of boilerplate.
208211
"""
209212
struct_members: dict[str, Schema] = {}
210213
if members:
211-
for k in members.keys():
214+
for i, (k, member) in enumerate(members.items()):
215+
if member is None:
216+
struct_members[k] = None # type: ignore
217+
continue
218+
212219
struct_members[k] = cls.member(
213220
id=id.with_member(k),
214-
target=members[k]["target"],
215-
index=members[k]["index"],
216-
member_traits=members[k].get("traits"),
221+
target=member["target"],
222+
index=i,
223+
member_traits=member.get("traits"),
217224
)
218225

219226
result = cls(
@@ -268,7 +275,6 @@ class MemberSchema(TypedDict):
268275
"""
269276

270277
target: Required[Schema]
271-
index: Required[int]
272278
traits: NotRequired[list["Trait | DynamicTrait"]]
273279

274280

‎packages/smithy-core/tests/unit/test_documents.py‎

Lines changed: 91 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,11 @@ def test_wrap_list_passes_schema_to_member_documents() -> None:
402402
list_schema = Schema.collection(
403403
id=id,
404404
shape_type=ShapeType.LIST,
405-
members={"member": {"target": target_schema, "index": 0}},
405+
members={
406+
"member": {
407+
"target": target_schema,
408+
}
409+
},
406410
)
407411
document = Document(["foo"], schema=list_schema)
408412
actual = document[0]._schema # pyright: ignore[reportPrivateUsage]
@@ -421,7 +425,11 @@ def test_setitem_on_list_passes_schema_to_member_documents() -> None:
421425
list_schema = Schema.collection(
422426
id=id,
423427
shape_type=ShapeType.LIST,
424-
members={"member": {"target": target_schema, "index": 0}},
428+
members={
429+
"member": {
430+
"target": target_schema,
431+
}
432+
},
425433
)
426434
document = Document(["foo"], schema=list_schema)
427435
document[0] = "bar"
@@ -440,7 +448,11 @@ def test_wrap_structure_passes_schema_to_member_documents() -> None:
440448
target_schema = Schema(id=ShapeID("smithy.api#String"), shape_type=ShapeType.STRING)
441449
struct_schema = Schema.collection(
442450
id=id,
443-
members={"stringMember": {"target": target_schema, "index": 0}},
451+
members={
452+
"stringMember": {
453+
"target": target_schema,
454+
}
455+
},
444456
)
445457
document = Document({"stringMember": "foo"}, schema=struct_schema)
446458
actual = document["stringMember"]._schema # pyright: ignore[reportPrivateUsage]
@@ -458,7 +470,11 @@ def test_setitem_on_structure_passes_schema_to_member_documents() -> None:
458470
target_schema = Schema(id=ShapeID("smithy.api#String"), shape_type=ShapeType.STRING)
459471
struct_schema = Schema.collection(
460472
id=id,
461-
members={"stringMember": {"target": target_schema, "index": 0}},
473+
members={
474+
"stringMember": {
475+
"target": target_schema,
476+
}
477+
},
462478
)
463479
document = Document({"stringMember": "foo"}, schema=struct_schema)
464480
document["stringMember"] = "spam"
@@ -478,8 +494,12 @@ def test_wrap_map_passes_schema_to_member_documents() -> None:
478494
id=id,
479495
shape_type=ShapeType.MAP,
480496
members={
481-
"key": {"target": STRING, "index": 0},
482-
"value": {"target": STRING, "index": 1},
497+
"key": {
498+
"target": STRING,
499+
},
500+
"value": {
501+
"target": STRING,
502+
},
483503
},
484504
)
485505
document = Document({"spam": "eggs"}, schema=map_schema)
@@ -496,8 +516,12 @@ def test_setitem_on_map_passes_schema_to_member_documents() -> None:
496516
id=id,
497517
shape_type=ShapeType.MAP,
498518
members={
499-
"key": {"target": STRING, "index": 0},
500-
"value": {"target": STRING, "index": 1},
519+
"key": {
520+
"target": STRING,
521+
},
522+
"value": {
523+
"target": STRING,
524+
},
501525
},
502526
)
503527
document = Document({"spam": "eggs"}, schema=map_schema)
@@ -517,47 +541,87 @@ def test_is_none():
517541
STRING_LIST_SCHEMA = Schema.collection(
518542
id=ShapeID("smithy.example#StringList"),
519543
shape_type=ShapeType.LIST,
520-
members={"member": {"target": STRING, "index": 0}},
544+
members={
545+
"member": {
546+
"target": STRING,
547+
}
548+
},
521549
)
522550
STRING_MAP_SCHEMA = Schema.collection(
523551
id=ShapeID("smithy.example#StringMap"),
524552
shape_type=ShapeType.MAP,
525553
members={
526-
"key": {"target": STRING, "index": 0},
527-
"value": {"target": STRING, "index": 1},
554+
"key": {
555+
"target": STRING,
556+
},
557+
"value": {
558+
"target": STRING,
559+
},
528560
},
529561
)
530562
SPARSE_STRING_LIST_SCHEMA = Schema.collection(
531563
id=ShapeID("smithy.example#StringList"),
532564
shape_type=ShapeType.LIST,
533-
members={"member": {"target": STRING, "index": 0}},
565+
members={
566+
"member": {
567+
"target": STRING,
568+
}
569+
},
534570
traits=[SparseTrait()],
535571
)
536572
SPARSE_STRING_MAP_SCHEMA = Schema.collection(
537573
id=ShapeID("smithy.example#StringMap"),
538574
shape_type=ShapeType.MAP,
539575
members={
540-
"key": {"target": STRING, "index": 0},
541-
"value": {"target": STRING, "index": 1},
576+
"key": {
577+
"target": STRING,
578+
},
579+
"value": {
580+
"target": STRING,
581+
},
542582
},
543583
traits=[SparseTrait()],
544584
)
545585
SCHEMA: Schema = Schema.collection(
546586
id=ShapeID("smithy.example#DocumentSerdeShape"),
547587
members={
548-
"booleanMember": {"target": BOOLEAN, "index": 0},
549-
"integerMember": {"target": INTEGER, "index": 1},
550-
"floatMember": {"target": FLOAT, "index": 2},
551-
"bigDecimalMember": {"target": BIG_DECIMAL, "index": 3},
552-
"stringMember": {"target": STRING, "index": 4},
553-
"blobMember": {"target": BLOB, "index": 5},
554-
"timestampMember": {"target": TIMESTAMP, "index": 6},
555-
"documentMember": {"target": DOCUMENT, "index": 7},
556-
"listMember": {"target": STRING_LIST_SCHEMA, "index": 8},
557-
"mapMember": {"target": STRING_MAP_SCHEMA, "index": 9},
558-
# Index 10 is set below because it's a self-referential member.
559-
"sparseListMember": {"target": SPARSE_STRING_LIST_SCHEMA, "index": 11},
560-
"sparseMapMember": {"target": SPARSE_STRING_MAP_SCHEMA, "index": 12},
588+
"booleanMember": {
589+
"target": BOOLEAN,
590+
},
591+
"integerMember": {
592+
"target": INTEGER,
593+
},
594+
"floatMember": {
595+
"target": FLOAT,
596+
},
597+
"bigDecimalMember": {
598+
"target": BIG_DECIMAL,
599+
},
600+
"stringMember": {
601+
"target": STRING,
602+
},
603+
"blobMember": {
604+
"target": BLOB,
605+
},
606+
"timestampMember": {
607+
"target": TIMESTAMP,
608+
},
609+
"documentMember": {
610+
"target": DOCUMENT,
611+
},
612+
"listMember": {
613+
"target": STRING_LIST_SCHEMA,
614+
},
615+
"mapMember": {
616+
"target": STRING_MAP_SCHEMA,
617+
},
618+
"structMember": None,
619+
"sparseListMember": {
620+
"target": SPARSE_STRING_LIST_SCHEMA,
621+
},
622+
"sparseMapMember": {
623+
"target": SPARSE_STRING_MAP_SCHEMA,
624+
},
561625
},
562626
)
563627
SCHEMA.members["structMember"] = Schema.member(

0 commit comments

Comments
 (0)