Object store item size fix#977
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a JSON deserialization crash that occurs when ObjectStore points to objects with large files by changing the Size property from int to ulong in ObjectMetadata.
- Changed
ObjectMetadata.Sizeproperty frominttoulongto handle large file sizes - Updated all related code to use
ulongfor size calculations and comparisons - Modified test assertions to use decimal literals for consistency with the new data type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/NATS.Client.ObjectStore/Models/ObjectMetadata.cs | Changed Size property from int to ulong |
| src/NATS.Client.ObjectStore/NatsObjStore.cs | Updated size variables and calculations to use ulong |
| tests/NATS.Client.ObjectStore.Tests/ObjectStoreTest.cs | Updated test assertions to use decimal literals and ulong casts |
| tests/NATS.Client.ObjectStore.Tests/WatcherTest.cs | Changed count variable from int to ulong |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Aligning with ADR-20: type ObjectInfo struct {
ObjectMeta
Bucket string `json:"bucket"`
NUID string `json:"nuid"`
// the total object size in bytes
Size uint64 `json:"size"` // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
ModTime time.Time `json:"mtime"`
// the total number of chunks
Chunks uint32 `json:"chunks"` // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
// as in http, <digest-algorithm>=<digest-value>
Digest string `json:"digest,omitempty"`
Deleted bool `json:"deleted,omitempty"`
} |
| var info = await store.GetInfoAsync("k1", cancellationToken: cancellationToken); | ||
|
|
||
| // Verify Size is ulong | ||
| ulong size = info.Size; // Should compile without error |
There was a problem hiding this comment.
since using large files won't be possible I just check the type here.
| [JsonPropertyName("chunks")] | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] | ||
| public int Chunks { get; set; } | ||
| public uint Chunks { get; set; } |
There was a problem hiding this comment.
turns out this one is not align with the spec either (ADR-20)
There was a problem hiding this comment.
I have long (int 64) in both java and .net v1. ADR has uint32 so I think this is fine.
mtmk
left a comment
There was a problem hiding this comment.
LGTM thanks @darkwatchuk
(I shall ask for one more review since I added a change too)
Disable style warnings in ObjectStoreTest to ensure compatibility with type validation tests
Thank you! |
|
sorry for the delay @darkwatchuk we should get this merged soon then I'm hoping to get it released some time this week. |
| [JsonPropertyName("chunks")] | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] | ||
| public int Chunks { get; set; } | ||
| public uint Chunks { get; set; } |
There was a problem hiding this comment.
I have long (int 64) in both java and .net v1. ADR has uint32 so I think this is fine.
* #863 Fix naming of consumer group attribute (#870) Signed-off-by: James Thompson <thompson.tomo@outlook.com> * #848 Tweak dependencies (#853) * #848 Tweak dependencies Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.Core.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.Core.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Make STJ explicit dependency for JetStream Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Update NATS.Client.JetStream.csproj Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Force sdk to 8.0.0 Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Add newer stj for net 6 Signed-off-by: James Thompson <thompson.tomo@outlook.com> --------- Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Add support for `Filter` and `Enrich` for OpenTelemetry activities (#859) * Add support for `Filter` and `Enrich` for OpenTelemetry activities * Make `internal` methods in `internal Telemetry` `public` * Fix package versions and whatnot * Remove `TracerProviderBuilderExtensions` * Include `Deserialize` in the receive activity * Revert back accidental change * Add `ParentContext` to `NatsInstrumentationContext` * Make `GetActivityContext` public to provide the ability to get context activity context * Make preprocessor directive more accurate * Revert .csproj formatting * Move public artifacts out of the `Internal` namespace/folder * Fix build script --------- Co-authored-by: Ziya Suzen <ziya@suzen.net> * Obsolete ReplyAsync method for NatsJsMsg (#839) * Obsolete ReplyAsync method * Obsolete ReplyAsync method on interface * #851 Move Serialization Interface into Abstractions (#858) * Tweaks Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Add missing dependencies Signed-off-by: James Thompson <thompson.tomo@outlook.com> * switch to system.memory Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Added missing using Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Reduce csproj file contents Signed-off-by: James Thompson <thompson.tomo@outlook.com> --------- Signed-off-by: James Thompson <thompson.tomo@outlook.com> * Release 2.7.0-preview.1 (#905) * #851 Move Serialization Interface into Abstractions (#858) * Obsolete ReplyAsync method for NatsJsMsg (#839) * Add support for `Filter` and `Enrich` for OpenTelemetry activities (#859) * #848 Tweak dependencies (#853) * #863 Fix naming of consumer group attribute (#870) * fix: `NatsInstrumentationOptions.Default` gets reset each time (#907) * initialize `NatsInstrumentationOptions.Default` once * `NatsInstrumentationContext.ActivityContext` doesn't need to be nullable * no need for `GetActivityContext` on NatsJSMsg * Release 2.7.0-preview.2 (#908) * fix: `NatsInstrumentationOptions.Default` gets reset each time (#907) * Also, fixes from main * Fix build warnings (#912) * fix: `NatsJSConsumer` never disposing receive activities (#911) * Fix `NatsJSConsumer` never disposing receive activities * Use `ActivityEndingMsgReader` to `NatsJSFetch` and `NatsJSOrderedConsume` * Consolidate activity ending message readers into one class * Run `dotnet format` * Release 2.7.0-preview.3 (#922) * fix: `NatsJSConsumer` never disposing receive activities (#911) * Merge from main * chore: rework NatsOpt.Default initialization (#921) * chore: rework NatsOpt.Default initialization * chore: removed unused using statement * Fix kv ttl interface (#909) KV TTL should only be allowed on Create and Purge * Release 2.7.0-preview.4 (#931) Fixes merged from main * Release 2.7.0-preview.5 (#944) Merge from main aligning with 2.6.8 release. * Fix JetStream publish retry defaults (#939) Decision to retry jetstream publish requests should be letft to the application since it depends on the delivery and durability requirements of their solution. * Fix publish 503 test (#958) With 2.7.x we have made the default to not re-publish on failure. * Release 2.7.0-preview.6 (#956) * Fix JetStream publish retry defaults (#939) * Merge from main * Release 2.7.0-preview.7 (#970) Merged from main * fix: handle 408 Requests Pending responses for fetch requests (#973) * Handle 408 Requests Pending responses for fetch requests * Remove redundant `Console.WriteLine` that was put there for testing * Release 2.7.0-preview.8 (#986) * fix: handle 408 Requests Pending responses for fetch requests (#973) * (merge from main) Object store item size fix (#977) * Fix Ad-Hoc JSON Serializer to use Default Options (#984) * Fix connection state for consume (#959) * Fix connection state for consume * Enhance connection state handling with NatsConnectionFailedException * Improve error handling for connection failures and add support for configurable 503 error thresholds in JetStream consumers * Add tests for connection failure handling and configurable 503 error thresholds in JetStream consumers * Fix format * Fix test * Update INatsJsConsumer to return INatsJsMsg (#1004) --------- Signed-off-by: James Thompson <thompson.tomo@outlook.com> Co-authored-by: James Thompson <thompson.tomo@outlook.com> Co-authored-by: Arad Alvand <aradalvand@gmail.com> Co-authored-by: Yeong Jong Lim <46077396+yeongjonglim@users.noreply.github.com> Co-authored-by: regnrat <66414875+regnrat@users.noreply.github.com>
ObjectStore ObjectMetaData.Size is currently an int and causes a JSON deserialization crash when pointing to an object store with large files.
Changing to a ulong fixes this issue.
e.g. someone has uploaded large files to this object store on demo.nats.io