Azure / azure-sdk-for-go

This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at:
https://docs.microsoft.com/azure/developer/go/
MIT License
1.64k stars 844 forks source link

Cosmos Go SDK fails to handle MaxInt64 in the QueryItemsResponse #22523

Closed xsuo2022 closed 8 months ago

xsuo2022 commented 8 months ago

Bug Report

What happened?

ealsur commented 8 months ago

Please see https://learn.microsoft.com/en-us/azure/cosmos-db/concepts-limits#per-item-limits

image

9223372036854775807 seems outside of the safe boundary of IEEE754 (9007199254740992) to work with the service. This will run into issues across other areas (like Portal interactions or other languages).

image

While in Go this could be fixed by removing the unmarshal call and change it to something else, the issue will still appear in other areas.

Storing these numbers as strings instead would be the recommendation.

We can certainly modify the Go SDK but that would not remove the problem across other service interactions in other languages/scenarios.

xsuo2022 commented 8 months ago

@ealsur Thanks for taking a look and providing the detailed docs! You are right, we also see the number handling issue with Azure Cosmos Portal, but the underlining data remains correct.

We can certainly modify the Go SDK but that would not remove the problem across other service interactions in other languages/scenarios.

Do you think you can introduce some option to configure? This is how spanner does it https://github.com/googleapis/google-cloud-go/blob/main/spanner/value.go#L120-L132

xsuo2022 commented 8 months ago

Hi Matias,

Thanks for taking a look and providing the detailed docs! You are right, we also see the number handling issue with Azure Cosmos Portal, but the underlying data remains correct.

We can certainly modify the Go SDK but that would not remove the problem

across other service interactions in other languages/scenarios.

Do you think you can introduce some option to configure? This is how spanner does it https://github.com/googleapis/google-cloud-go/blob/main/spanner/value.go#L120-L132

Best, Xin

On Tue, Mar 5, 2024 at 1:09 PM Matias Quaranta @.***> wrote:

Please see https://learn.microsoft.com/en-us/azure/cosmos-db/concepts-limits#per-item-limits

image.png (view on web) https://github.com/Azure/azure-sdk-for-go/assets/1633401/f558688d-4239-4c92-aa43-1fa3ad43e14b

9223372036854775807 seems outside of the safe boundary of IEEE754 https://www.rfc-editor.org/rfc/rfc8259#ref-IEEE754 (9007199254740992) to work with the service. This will run into issues across other areas (like Portal interactions or other languages).

image.png (view on web) https://github.com/Azure/azure-sdk-for-go/assets/1633401/86eda2ce-4b98-43aa-a083-624dfd0606fe

While in Go this could be fixed by removing the unmarshal call and change it to something else, the issue will still appear in other areas.

Storing these numbers as strings instead would be the recommendation.

We can certainly modify the Go SDK but that would not remove the problem across other service interactions in other languages/scenarios.

— Reply to this email directly, view it on GitHub https://github.com/Azure/azure-sdk-for-go/issues/22523#issuecomment-1979639671, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2AOMRJUGOXFVTYMK3I6CRTYWYYBRAVCNFSM6AAAAABEHYHOJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZZGYZTSNRXGE . You are receiving this because you authored the thread.Message ID: @.***>

ealsur commented 8 months ago

the underlining data remains correct.

Right, the problem is that it is not consistent across. That's why it's safer to not use them.

Do you think you can introduce some option to configure?

One solution would be to simply remove the unmarshal and attempt to simply return the byte[] of the document (unsure exactly what the implementation would look like right now). If that can be done (like in ReadItem), then at least from the Go SDK perspective, you'd be ok. But the problem is still there.

In the future when things like Cross Partition Query support is added with things like aggregations (that need to happen on the client), then this will probably happen again.

For Json serialization, our Cosmos SDK uses azruntime.UnmarshalAsJSON, which uses json.Unmarshal under the covers. I don't know if "encoding/json" has any such setting that you could customize.

CurryFishBalls9527 commented 8 months ago

In the encoding/json package, there are decoder and unmarshaller, the formal gives you a little bit more control.
For example, it has https://pkg.go.dev/encoding/json#Decoder.UseNumber, which config the Decoder to unmarshal a number into an interface{} as a Number instead of as a float64.

xsuo2022 commented 8 months ago

Thanks @CurryFishBalls9527!

@ealsur Do you think this is something you can introduce to the sdk?

ealsur commented 8 months ago

I can add it to the backlog. As I mentioned, right now our SDK does not invoke json directly but consumes it from azruntime.

But even if we do this, please see my other comments, using these Numbers will have further potential repercussions outside of this SDK.