feat(compass-collection): Follow-ups tweaks to convertBSONToPrimitive - CLOUDP-351455

[jcobis]

Description

Followups addressing non-blocking comments from #7439

• Throw error instead of use fallback value for binary sample value edge case (that we do not expect to encounter)
• Cast instead of use instanceof

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• If this change updates the UI, screenshots/videos are added and a design review is requested
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[Copilot]

Pull Request Overview

This PR implements follow-up improvements to the convertBSONToPrimitive function based on code review feedback. The changes enhance error handling and improve type checking by using BSON type properties instead of instanceof checks.

• Replace instanceof checks with _bsontype property-based type detection
• Throw error for Binary data edge case instead of returning fallback value
• Add comprehensive test coverage for the Binary data error scenario

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
packages/compass-collection/src/transform-schema-to-field-info.ts	Refactored BSON type detection using _bsontype property and added error handling for Binary data
packages/compass-collection/src/transform-schema-to-field-info.spec.ts	Added test case to verify Binary data error throwing behavior

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[kraenhansen]

This seems a bit backward to me. If you're moving away from instanceof checks, I'd prefer leaning into type guards based on a union of expected BSON types. Actually surprised "bson" doesn't already have this union defined 🤔

I put together a TS playground to exemplify what I'm talking about.

[addaleax]

Well, they do expose the BSONValue class, so you could make this

Suggested change

	/**
	* Helper to cast BSON objects to their specific types
	*/
	function castBSONValue<T>(value: unknown): T {
	return value as T;
	}
	/**
	* Helper to cast BSON objects to their specific types
	*/
	function castBSONValue<T extends BSONValue>(value: BSONValue): T {
	return value as T;
	}

[kraenhansen]

The BSONValue from typescript is the abstract super-class? It adds no constrains to the _bsontype field beyond string ... seems much less usable than a union over the concrete implementations of that super-class.

The original and suggested code effectively use none of the type guarantees provided by "bson": There's nothing linking the switch-case statements to the particular BSONValue - might as well implement it in JavaScript at that point 🤷

[addaleax]

Yes, that's correct, and I agree that it would be useful to have a union of all possible BSON values available somewhere 🙂

I think we effectively have that elsewhere in Compass, though

[kraenhansen]

we effectively have that elsewhere in Compass

That would be a good candidate for reuse, I'm not aware of such utility?

[addaleax]

Mentioned it below:

with TypeCastMap from the hadron-type-checker package.

It's not perfect but it should work here

[jcobis]

Went with the suggestion to use BSONValue for now, which allowed me to remove that cast. Let me know what you think

(Edit: just refreshed this page and saw the new comments since)

[addaleax]

Well, they do expose the BSONValue class, so you could make this

Suggested change

	/**
	* Helper to cast BSON objects to their specific types
	*/
	function castBSONValue<T>(value: unknown): T {
	return value as T;
	}
	/**
	* Helper to cast BSON objects to their specific types
	*/
	function castBSONValue<T extends BSONValue>(value: BSONValue): T {
	return value as T;
	}

[kraenhansen]

My specific concern is this type assertion and I've suggested better and more type-safe ways to express this.

[addaleax]

and I've suggested better

Gotta disagree with this one. It's not a better suggestion.

I think we're in alignment that it would be good to have the bson library expose a type that fully describes possible BSON types. That's a fair request, but it's not something we can handle in this PR on its own, it requires a change to the BSON library itself.

If you currently want something that describes all possible BSON types, I'd go with TypeCastMap from the hadron-type-checker package.