Skip to content

fix: int32→int64 for BSON property values#71

Merged
ako merged 1 commit intomendixlabs:mainfrom
engalar:fix/bson-int64
Apr 1, 2026
Merged

fix: int32→int64 for BSON property values#71
ako merged 1 commit intomendixlabs:mainfrom
engalar:fix/bson-int64

Conversation

@engalar
Copy link
Copy Markdown
Contributor

@engalar engalar commented Apr 1, 2026

Summary

  • All BSON property values changed from int32 to int64 to match Studio Pro type cache expectations
  • Array markers (first element of bson.A) kept as int32 — verified against Studio Pro-generated .mxunit files
  • Added safeInt64() helper for settings serialization
  • Removed 26-line debug log block in writer_widgets_custom.go
  • Fixed 4 comments that incorrectly referenced int64 for array markers
  • Added golden .mxunit files from Studio Pro for roundtrip testing

Root Cause

Studio Pro crashes with Sequence contains no matching element when BSON property values are int32 — its type cache matches by BSON wire type (0x10 vs 0x12).

Test plan

  • go build passes
  • go test ./sdk/mpr/ passes
  • Verified array marker type (int32) against Studio Pro BSON using programmatic inspection of golden files
  • Studio Pro opens project without crash

🤖 Generated with Claude Code

Property values must be int64 in BSON — Studio Pro's type cache matches
by BSON type and crashes with "Sequence contains no matching element"
on int32. Array markers remain int32 (verified against Studio Pro BSON).

- Convert all property values from int32 to int64 across writer_*.go
- Add safeInt64() helper for settings serialization
- Remove debug logging left in writer_widgets_custom.go
- Fix 4 comments that incorrectly referenced int64 for array markers
- Add golden .mxunit files from Studio Pro for roundtrip testing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2026

AI Code Review

Review Summary

This PR fixes a critical compatibility issue where Studio Pro crashes due to BSON type mismatches. The changes are focused, well-verified, and follow the project's patterns.

What Looks Good

  • Clear root cause explanation: Identifies that Studio Pro's type cache matches by BSON wire type (0x10 vs 0x12), causing "Sequence contains no matching element" crash when property values are int32 instead of int64
  • Precise fix: Changes only BSON property values to int64 while keeping array markers as int32 (verified against Studio Pro-generated .mxunit files)
  • Consistent application: Updates applied uniformly across all relevant writer files (microflows, pages, widgets, settings, etc.)
  • Helpful additions:
    • Added safeInt64() helper for settings serialization (replacing removed safeInt32)
    • Removed 26-line debug log block in writer_widgets_custom.go
    • Fixed 4 incorrect comments about array marker types
    • Added golden .mxunit files from Studio Pro for roundtrip testing verification
  • Verification approach: Programmatic inspection of golden files confirms array markers remain int32 while property values are int64
  • Test coverage: go build and go test ./sdk/mpr/ passing shows no regressions introduced

Moderate Issues

  • safeInt64() simplicity: The helper function is a simple cast (return int64(v)) without the clamping logic present in the previous safeInt32(). While int64's larger range makes overflow less likely in practice, it's worth noting this behavioral change. However, given the context (settings values like port numbers, heap sizes), clamping may not be necessary.

Minor Issues

  • Test completeness: The test plan shows "[ ] Studio Pro opens project without crash" as unverified. While understandable (requires manual verification or specific test environment), this is the ultimate validation of the fix.

Recommendation

Approve - This PR correctly addresses a critical compatibility issue with Studio Pro. The changes are:

  1. Focused on a single root cause (BSON integer type mismatch)
  2. Properly verified against Studio Pro-generated files
  3. Consistently applied across the codebase
  4. Accompanied by appropriate test artifacts (golden files)
  5. Clean (removing debug code, fixing misleading comments)

The fix maintains the required distinction between property values (int64) and array markers (int32) as verified against Studio Pro output, resolving the "Sequence contains no matching element" crash. The one pending manual verification item (Studio Pro opening projects) is understandable for this type of fix and doesn't block approval given the strong evidence provided.


Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Copy link
Copy Markdown
Collaborator

@ako ako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

This is a clean, well-scoped rework of PR #66. All the issues from my earlier review have been addressed:

PR #66 Issue Status
Debug logging (log.Printf("SERIALIZE CHECK: ...")) Fixed — removed
Inconsistent array marker treatment Fixed — all array markers consistently int32, only property values changed to int64
Comment/code mismatch Fixed — comments correctly reference int32 for array markers
Bundled unrelated changes (CREATE OR REPLACE PAGE) Fixed — split out, single concern

What looks good

  • Consistent rule: property values → int64, array markers → int32. Applied uniformly across all 14 writer files.
  • Single commit, single concern — exactly what the repo's commit checklist asks for.
  • Test updated (writer_rest_test.go) — the Timeout assertion correctly changed to int64.
  • Verified against Studio Pro BSON per the test plan.

Minor items (non-blocking)

  1. safeInt64 is a no-opfunc safeInt64(v int) int64 { return int64(v) }. On 64-bit Go this is a widening cast that can never fail. The function name implies safety/clamping that no longer exists. Consider inlining int64(v) directly or renaming to toInt64. Very minor.

  2. Golden .mxunit files have no test functions — the 4 testdata files are added but no Go test reads them yet. The PR description says they're "for roundtrip testing" — if a follow-up is planned, that's fine, but standalone they're unused.

  3. Studio Pro test unchecked — the test plan shows [ ] Studio Pro opens project without crash. This is the most important validation for this fix. Would be good to confirm before merging.

Looks good overall. This is ready to merge once the Studio Pro test is confirmed.

@ako ako merged commit 295c757 into mendixlabs:main Apr 1, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants