Skip to content

Alternative PR to address uniqueness ordering and properties, introducing handle(bson) - #152

Open
hhaensel wants to merge 5 commits into
JuliaDatabases:masterfrom
hhaensel:hh-uniqueness-ordering-properties
Open

hhaensel wants to merge 5 commits into
JuliaDatabases:masterfrom
hhaensel:hh-uniqueness-ordering-properties

Conversation

@hhaensel

Copy link
Copy Markdown

This PR is in most parts identical to #151 but it makes DEFAULT_DICT_TYPE a true constant and converts all property calls to bson.handle to calls like handle(bson), where bson is any variable of type BSON.
This is inspired by #146.

@ancapdev

Copy link
Copy Markdown
Collaborator

It might be better to separate these changes in two PRs, but some comments here.

Property based data access

The getproperty() change is API breaking, which I think deserves more careful consideration. At minimum it needs to be released as a major vesion change. I'd also suggest either the API is changed to this new model, or it's not, not some halfway point where we pretend to support handle access by property name and it works conditional on the data held in the document.

Dict type

Have you considered surfacing this as OrderedDict(::BSON), where the materialization into the dictionary is applied with this dictionary type recursively? This could be implemented as a package extension, rather than a hard dependency. Maybe the dictionary type could be a parameter to BSONIterator as well, would seem natural and ergonomic.

@hhaensel

hhaensel commented Apr 26, 2026 •

Copy link
Copy Markdown
Author

Thanks for answering:
I am well aware of the implications of the proposed changes and the PRs were rather meant to showcase their usefulness/feasibility than to be a ready solution.
My first PR was a copy of my personal extension that was sufficient for my use case. When I thought for a bit longer I disliked my halfway approach and therefore submitted #152 .
I could, indeed, imagine that all the proposed changes go into a version 0.11.0. It would be a similar move as JSON did when they published v1.0, where they replaced Dict with the ordered JSON.Object. People who want to make use of the new notations would need to update and make sure that they adapt their programs with respect to the handle field.
I am open for any solution and I could prepare 2 or 3 different PRs depending on your preference. Below I list the changes that we may want to distribute to different PRs:

  • introduce getindex with a second optional index that contains a Dict type that is used recursively
  • introduce getindex with a Symbol key, also with an optional second index. This method makes sure to read the last entry
  • introduce merge!
  • introduce getproperty (setproperty! is not critical as that can be solved by type dispatch)

Leaving both getindex methods with String key and Symbol key could be interesting. People who are sure that they don't overwrite or call merge! explicitly can benefit from less iteration, if String keys always returns the first entry.

Questions to be answered_

  • Should getpropert use the Symbol key or the String key method?
  • Should OrderedDict be the default type for all getindex methods?

Concerning your question with BSONIterator, you're probably right, but I'm at the beginning of using the package and didn't have the need to use BSONIterator yet.

Concerning your proposal of OrderedDict(::BSON) is indeed tempting, why not defining
OrderedDict(::BSON; recursive::Bool = true)

EDIT:

One more question:
Chosing a version that adds an extension MongocOrderedCollectionsExt is perhaps a good interim solution. The only problem is that Mongoc currently supports julia 1.6 while package extensions are supported from version 1.9 onwards. How would we deal with that? There is the possibility of adding julia 1.6-1.8 support via the package Requires.jl (as I have done for Stipple and some other packages from the GenieFramework), but that adds about the same amount of code as OrderedCollections would...
But if I would add such an extension, should I then default the output of getindex(::BSON, ::Symbol) to OrderedDict?

@quinnj quinnj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fresh review of unchanged head e98171fad79aaa061fb66b9c2b186b44ae792b18 found two mutation/ownership defects and one selector defect, reproduced on both minimum Julia 1.6.7 and current 1.13.1. The nine focused assertions give five passes and four failures on each version; the nested OrderedDict support itself passes. No MongoDB server is needed for these BSON reproductions. The fork branch was left unchanged. No Actions run or check results are available for this exact head, so I am not claiming full CI validation.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

Comment thread src/bson.jl
vv = document[index]
# replace the dummy value by the real value
vv[k_pos] = value
bson_reinit(document)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P1] Preserve the document when an assigned value cannot be encoded

d = Mongoc.BSON("a" => Int32(1), "b" => "keep"); d.a = big(2) throws a MethodError after this bson_reinit, leaving d empty and losing both original fields. I reproduced this on Julia 1.6.7 and 1.13.1. Build and validate the replacement in a separate BSON first, then commit it to the original only after successful encoding. Otherwise an ordinary unsupported value destroys unrelated data.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

Comment thread src/bson.jl
end

function Base.setproperty!(document::BSON, key::Symbol, value::Ptr{Nothing})
setfield!(document, :handle, value)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P1] Limit native-handle assignment to the handle property

This overload ignores key. d.unrelated = C_NULL silently replaces the live native handle even though the caller did not write the handle property. I reproduced the pointer replacement on Julia 1.6.7 and 1.13.1, restoring the saved live pointer before any native operation. The original allocation becomes unreachable and later document access operates on the replacement pointer. Restrict this compatibility path to key === :handle; other pointer-valued document properties should follow normal value validation.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

Comment thread src/bson.jl
vv = Vector{Any}(undef, n)
while(bson_iter_next(iter_ref))
i += 1
if i ∈ index

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Handle Boolean selectors before treating indices as positions

Mongoc.BSON("a" => Int32(1), "b" => Int32(2))[[false, true]] raises UndefRefError on Julia 1.6.7 and 1.13.1. Membership treats true as position 1, so slot 2 stays uninitialized, but the final Boolean indexing selects slot 2. Normalize Boolean masks to positions, or reject them explicitly if this helper only supports integer positions.

AI disclosure: This work was prepared with assistance from OpenAI Codex.

This branch has not been deployed

No deployments
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.

3 participants