SY-3797: Oracle Prep 12 - Refactor Tasks and Devices to Move JSON Instead of Strings#2009
Conversation
…8-improve-type-safety-of-device-queries
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rc #2009 +/- ##
==========================================
+ Coverage 53.69% 53.72% +0.03%
==========================================
Files 2464 2464
Lines 151409 151474 +65
Branches 7096 7116 +20
==========================================
+ Hits 81298 81381 +83
+ Misses 67645 67625 -20
- Partials 2466 2468 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…://github.com/synnaxlabs/synnax into sy-3797-use-proper-json-for-tasks
pjdotson
left a comment
pjdotson
left a comment
There was a problem hiding this comment.
Should we force the JSON to be a JSON object or should it be allowed to be any type of JSON (string, null, number, boolean, array, or object)? While I don't see us internally using anything other than objects, it seems like it might be better to allow anything to be satisfied for a more expansive API, and preventing some sort of migrations / code changes if we, say, want to allow customers to store task configurations as an array. I would push
| f.setConfigStatus(ctx, t, xstatus.VariantError, err.Error()) | ||
| return nil, true, err | ||
| } | ||
| if err := json.Unmarshal(cfgJSON, &cfg); err != nil { |
There was a problem hiding this comment.
so we have to marshal and unmarshal the JSON here? will we have to do this every time we
write a new Go driver? Why isn't the config just already usable?
There was a problem hiding this comment.
like why don't we try to type assert a t.Config as a TaskConfig instead so we don't
have to marshal / unmarshal?
There was a problem hiding this comment.
I think the reality is that we're going to store this as some sort of generic protobuf type in the future. pb.Any or pb.Struct and then we'll use oracle generated code to convert it into a concrete type. If we make the field any we still can't do a type assertion because the json/msgpack codec will automatically decode the interface into the underlying type of map[string]any. The long term solution here is to make this a generic and have proper pb-based encode/decode mechanisms, but that it is out of scope here.
The objective with this PR was to standardize JSON representation so we could have a consistent oracle type.
So yes, this is necessary for now but will be replaced in the future.
There was a problem hiding this comment.
I added an unmarshal helper that abstracts this away from the caller for now.
| model: str = "" | ||
| configured: bool = False | ||
| properties: str = "" | ||
| properties: dict[str, Any] = {} |
There was a problem hiding this comment.
is there not a better JSON type or something than having to use Any?
There was a problem hiding this comment.
I think this is the most narrow, reasonable type for now. I think in the future this should use a properly validate generics structure.
…8-improve-type-safety-of-device-queries
…8-improve-type-safety-of-device-queries
…://github.com/synnaxlabs/synnax into sy-3797-use-proper-json-for-tasks
Looks like this comment got cut off I don't see any world in which using a non-object format is of considerable value for the end user. There's definitely no reasonable case where you would use a primitive, and I think that the harm in having an array based config is negligible. By enforcing an object we dramatically simplify the set of types we need to support in typescript, Python, and C++. |
…7-use-proper-json-for-tasks
Okay. I agree that the primitive / array based aren't that useful. However I don't think it is the case that it really simplifies the typing. IE the standard JSON type in Python is this (which we should be using instead of from typing import TypeAlias
JSON: TypeAlias = (
dict[str, "JSON"]
| list["JSON"]
| str
| int
| float
| bool
| None
)so we would then want the types in our task config to be All of which to say I think it would actually be easier to support with good typing any JSON value, and right now the state of the PR does not completely have good typing in client libraries. |
2 participants
Issue Pull Request
Linear Issue
SY-3797
Description
Basic Readiness
Greptile Summary
This PR refactors task configs and device properties throughout the entire stack to use native JSON objects instead of JSON-encoded strings, improving type safety and API ergonomics.
Key Changes:
google.protobuf.Structinstead of string fields fordevice.propertiesandtask.configbinary.MsgpackEncodedJSONtype in Go for backwards-compatible msgpack decoding (handles both old string format and new map format)x/cpp/json/struct.h) for converting between nlohmann::json and protobuf StructImpact:
MsgpackEncodedJSONConfidence Score: 4/5
MsgpackEncodedJSONdecoder working correctly with existing data, and 3) extensive integration testing is needed to ensure all client-server interactions work correctly across the Python, TypeScript, and C++ clientsx/go/binary/codec.go(backwards compatibility logic),core/pkg/api/grpc/device/handler.goandcore/pkg/api/grpc/task/handler.go(translation layer), and integration test resultsImportant Files Changed
propertiesfield from string togoogle.protobuf.Structfor proper JSON handlingconfigfield from string togoogle.protobuf.Structfor proper JSON handlingMsgpackEncodedJSONtype for backwards-compatible msgpack decoding of JSON stringsbinary.MsgpackEncodedJSONfor native JSON supportbinary.MsgpackEncodedJSONfor native JSON supportmap[string]anyandstructpb.Structmap[string]anyandstructpb.Structgoogle.protobuf.Structx::json::from_structandx::json::to_structutilitiesx::json::from_structandx::json::to_structutilitiesmerge_device_propertiesto usex::json::jsoninstead of JSON stringsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Client: Python/TS/C++] -->|JSON Objects| B[gRPC Layer] B -->|google.protobuf.Struct| C[Handler Translation] C -->|map string any| D[Go Service Layer] D -->|binary.MsgpackEncodedJSON| E[Msgpack Storage] E -->|Backwards Compatible| F{Old Format?} F -->|JSON String| G[Parse String to Map] F -->|Map| H[Use Directly] G --> I[Unified Map Representation] H --> I I -->|Reverse Flow| D D -->|map string any| C C -->|google.protobuf.Struct| B B -->|JSON Objects| A style E fill:#e1f5ff style I fill:#d4edda style F fill:#fff3cdLast reviewed commit: 99f760b