From 87baaceb43ef3491385375caf0dac85c5ed9d758 Mon Sep 17 00:00:00 2001
From: Thomas Boyer-Chammard <49786685+thomas-bc@users.noreply.github.com>
Date: Thu, 30 Jan 2025 16:12:10 -0800
Subject: [PATCH] Add struct member index logic to JSON loader (#188)

* Add struct member index parsing to JSON loader

* Add test case

* really looks like a word to me

* Review fix: improve readability
---
 .github/actions/spelling/expect.txt           |  1 +
 src/fprime_gds/common/loaders/json_loader.py  | 15 +++++++--
 .../resources/RefTopologyDictionary.json      | 32 +++++++++++++++++++
 .../common/loaders/test_json_loader.py        | 10 ++++++
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt
index f23f80aa..786365d4 100644
--- a/.github/actions/spelling/expect.txt
+++ b/.github/actions/spelling/expect.txt
@@ -203,6 +203,7 @@ mcternan
 MDFILE
 memb
 millis
+misordered
 MML
 moz
 msc
diff --git a/src/fprime_gds/common/loaders/json_loader.py b/src/fprime_gds/common/loaders/json_loader.py
index c55b10a5..2f3f6f53 100644
--- a/src/fprime_gds/common/loaders/json_loader.py
+++ b/src/fprime_gds/common/loaders/json_loader.py
@@ -178,7 +178,10 @@ def construct_serializable_type(
             SerializableType: The constructed serializable type.
 
         """
-        struct_members = []
+        # Note on struct_members: the order of the members list matter when calling construct_type() below.
+        # It should be ordered by incrementing index which corresponds to the order in the FPP declaration
+        # The JSON dictionary ordering is not guaranteed, so we use a dict() to sort by index below.
+        struct_members = {}
         for name, member_dict in qualified_type.get("members").items():
             member_type_dict = member_dict["type"]
             member_type_obj = self.parse_type(member_type_dict)
@@ -197,11 +200,17 @@ def construct_serializable_type(
                 member_type_obj.FORMAT if hasattr(member_type_obj, "FORMAT") else "{}"
             )
             description = member_type_dict.get("annotation", "")
-            struct_members.append((name, member_type_obj, fmt_str, description))
+            member_index = member_dict["index"]
+            if member_index in struct_members:
+                raise KeyError(
+                    f"Invalid dictionary: Duplicate index {member_index} in serializable type {type_name}"
+                )
+            struct_members[member_index] = (name, member_type_obj, fmt_str, description)
 
+        # Construct the serializable type with list of members sorted by index
         ser_type = SerializableType.construct_type(
             type_name,
-            struct_members,
+            [struct_members[i] for i in sorted(struct_members.keys())],
         )
         self.parsed_types[type_name] = ser_type
         return ser_type
diff --git a/test/fprime_gds/common/loaders/resources/RefTopologyDictionary.json b/test/fprime_gds/common/loaders/resources/RefTopologyDictionary.json
index 70d8510b..175195bd 100644
--- a/test/fprime_gds/common/loaders/resources/RefTopologyDictionary.json
+++ b/test/fprime_gds/common/loaders/resources/RefTopologyDictionary.json
@@ -8,6 +8,38 @@
     "dictionarySpecVersion" : "1.0.0"
   },
   "typeDefinitions" : [
+    {
+      "kind" : "struct",
+      "qualifiedName" : "Ref.TestMisorderedStructIndexes",
+      "members" : {
+        "ThisIsOne" : {
+          "type" : {
+            "name" : "U32",
+            "kind" : "integer",
+            "size" : 32,
+            "signed" : false
+          },
+          "index" : 1
+        },
+        "ThisIsZero" : {
+          "type" : {
+            "name" : "U32",
+            "kind" : "integer",
+            "size" : 32,
+            "signed" : false
+          },
+          "index" : 0
+        },
+        "ThisIsTwo" : {
+          "type" : {
+            "name" : "Ref.PacketRecvStatus",
+            "kind" : "qualifiedIdentifier"
+          },
+          "index" : 2
+        }
+      },
+      "annotation" : "This type does not exist in Ref and is only used for GDS internal testing"
+    },
     {
       "kind" : "struct",
       "qualifiedName" : "Ref.PacketStat",
diff --git a/test/fprime_gds/common/loaders/test_json_loader.py b/test/fprime_gds/common/loaders/test_json_loader.py
index 88f76401..2f5ee69e 100644
--- a/test/fprime_gds/common/loaders/test_json_loader.py
+++ b/test/fprime_gds/common/loaders/test_json_loader.py
@@ -100,6 +100,16 @@ def test_construct_serializable_type(loader):
     assert ref_choice_pair.MEMBER_LIST[1][2] == "{}"
 
 
+def test_struct_with_unordered_members(loader):
+    misordered_member = loader.parse_type(
+        {"name": "Ref.TestMisorderedStructIndexes", "kind": "qualifiedIdentifier"}
+    )
+    assert issubclass(misordered_member, SerializableType)
+    assert misordered_member.MEMBER_LIST[0][0] == "ThisIsZero"
+    assert misordered_member.MEMBER_LIST[1][0] == "ThisIsOne"
+    assert misordered_member.MEMBER_LIST[2][0] == "ThisIsTwo"
+
+
 def test_construct_primitive_types(loader):
     i32_type = loader.parse_type(
         {"name": "I32", "kind": "integer", "size": 32, "signed": True}