Skip to content

Implement BSON Marshaler support #142

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ryclarke
Copy link
Contributor

@ryclarke ryclarke commented Aug 6, 2024

Adds implementation of bson.Marshaler interface to Sets.
Also addresses a locking inconsistency in UnmarshalJSON.

BSON support is important for our use cases, and should be a good improvement to this library's capabilities.

@@ -291,9 +291,25 @@ func (t *threadSafeSet[T]) MarshalJSON() ([]byte, error) {
}

func (t *threadSafeSet[T]) UnmarshalJSON(p []byte) error {
t.RLock()
t.Lock()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UnmarshalJSON should be a write lock - unlikely to cause issues in practice (with a newly-initialized Set), but an inconsistency nonetheless.

@deckarep
Copy link
Owner

deckarep commented Aug 6, 2024

Hi @ryclarke,

Thanks for the contribution and nice catch on the locking issue for UnmarshalJSON.

I want to make you aware of this issue if you're not already: #122
This affects the regular MarshalJSON/UnmarshalJSON api where there exists an edge case causing a panic. It will likely require a larger refactor of this code and may have implications for the BSON marshaling/unmarshaling as well.

See the issue to get more context but I have no problem in general adding support for BSON.

@ryclarke
Copy link
Contributor Author

ryclarke commented Aug 6, 2024

Thanks for the heads up - time permitting I may be able to help implement a fix for that bug in a separate PR

@ryclarke ryclarke force-pushed the feature/marshal-bson branch from 82746cc to fff59c0 Compare August 7, 2024 14:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants