Conversation
#1861) * fix: prevent defaults being set to undefined, and interpret numbers and enums as strings. * chore: Auto-format JavaScript files with Prettier
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures incoming protobufjs messages are converted to plain JavaScript objects with default values and consistent scalar representations before invoking the message handler, with optional type resolution based on the active field. Sequence diagram for protobufjs message handling and conversionsequenceDiagram
participant CoreServer
participant g_core_sock
participant MessageDispatcher
participant ProtobufType
participant Handler_f
CoreServer->>g_core_sock: incoming_message
g_core_sock->>MessageDispatcher: on_message(msg, msg_info, which_field, correlation_id)
MessageDispatcher->>MessageDispatcher: set g_ctx_next = ctx
alt msg is not null
MessageDispatcher->>MessageDispatcher: resolve_type = msg_info.type
alt which_field is set
MessageDispatcher->>MessageDispatcher: actual_entry = find in g_msg_by_id by field_name
alt actual_entry found
MessageDispatcher->>MessageDispatcher: resolve_type = actual_entry.type
end
end
alt resolve_type is set
MessageDispatcher->>ProtobufType: toObject(msg, {defaults: true, longs: String, enums: String})
ProtobufType-->>MessageDispatcher: msg_plain_object
end
end
MessageDispatcher->>Handler_f: f(msg_plain_object_or_original)
Handler_f-->>MessageDispatcher: handler_result
Flow diagram for protobufjs message to plain object conversionflowchart TD
A[start on_message callback] --> B{msg is not null}
B -->|no| H[call f with original msg]
B -->|yes| C[set resolve_type = msg_info.type]
C --> D{which_field is set}
D -->|no| G{resolve_type is set}
D -->|yes| E[actual_entry = find in g_msg_by_id by field_name]
E --> F{actual_entry found}
F -->|no| G{resolve_type is set}
F -->|yes| C2[set resolve_type = actual_entry.type]
C2 --> G
G -->|no| H[call f with original msg]
G -->|yes| I["msg = resolve_type.toObject(msg, defaults true, longs String, enums String)"]
I --> J[call f with converted plain object msg]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
Object.values(g_msg_by_id).find(...)lookup on every message may introduce unnecessary overhead; consider caching afield_name → typemap or storingtypedirectly wherewhich_fieldis determined. - Converting
msgin-place from a protobufjs message to a plain object changes the type seen byf(msg); if any existing handlers rely on protobuf-specific methods/behavior, it may be safer to either gate this behavior behind a flag or pass a separate converted object instead of mutatingmsg.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Object.values(g_msg_by_id).find(...)` lookup on every message may introduce unnecessary overhead; consider caching a `field_name → type` map or storing `type` directly where `which_field` is determined.
- Converting `msg` in-place from a protobufjs message to a plain object changes the type seen by `f(msg)`; if any existing handlers rely on protobuf-specific methods/behavior, it may be safer to either gate this behavior behind a flag or pass a separate converted object instead of mutating `msg`.
## Individual Comments
### Comment 1
<location path="web/datafed-ws.js" line_range="2468-2471" />
<code_context>
+ if (actual_entry) resolve_type = actual_entry.type;
+ }
+ if (resolve_type) {
+ msg = resolve_type.toObject(msg, {
+ defaults: true,
+ longs: String,
+ enums: String,
+ });
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Double-check that converting longs/enums to strings here doesn’t break downstream expectations.
This changes `msg` from a protobuf Message into a plain object with `longs` and `enums` as strings. Any consumer relying on numeric enums, Long instances, or protobuf-specific methods may break subtly. Consider gating this conversion behind a feature flag/option, or verify all downstream consumers are updated to handle the new string-based shape.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| msg = resolve_type.toObject(msg, { | ||
| defaults: true, | ||
| longs: String, | ||
| enums: String, |
There was a problem hiding this comment.
issue (bug_risk): Double-check that converting longs/enums to strings here doesn’t break downstream expectations.
This changes msg from a protobuf Message into a plain object with longs and enums as strings. Any consumer relying on numeric enums, Long instances, or protobuf-specific methods may break subtly. Consider gating this conversion behind a feature flag/option, or verify all downstream consumers are updated to handle the new string-based shape.
Update Staging
Summary by Sourcery
Enhancements: