firebase / firebase/firebase-admin-go

Realtime Database: GetIfChanged and GetWithETag will not return etag if json.Unmarshal returns an error

Open
#568 0 comments 0 reactions 0 assignees View on GitHub
api: database
Dominant language
Go
Stars
1.3k
Forks
274
Avg merge
10h 39m
Merged PRs (30d)
2

Description

**Describe your environment**
* Operating System version: macOS Ventura 13.0.1
* Firebase SDK version: firebase.google.com/go/v4 v4.10.0
* Library version: v4.10.0
* Firebase Product: database

**Describe the problem**
json.Unmarshal() is robust to schema issues - it will populate as much of the value as possible.
https://pkg.go.dev/encoding/json#Unmarshal
> If a JSON value is not appropriate for a given target type, or if a JSON number overflows the target type, Unmarshal skips that field and completes the unmarshaling as best it can. If no more serious errors are encountered, Unmarshal returns an UnmarshalTypeError describing the earliest such error.

However, GetWithETag treats all errors from json.Unmarshall as catastrophic failures: it returns an empty etag despite the value v being populated by json.Unmarshall().
https://github.com/firebase/firebase-admin-go/blob/74c9bd5edece6044ba365a04c32c00074006dd7c/db/ref.go#L93

GetIfChanged is similar. It returns an empty etag, and also fails to declare if the data has changed.
https://github.com/firebase/firebase-admin-go/blob/74c9bd5edece6044ba365a04c32c00074006dd7c/db/ref.go#L130

The consequences are:
* The client doesn't receive a valid etag from either method: every subsequent call to GetIfChanged will retrieve the full reference data (expensive)
* GetIfChanged will never return changed=true: if the client code uses changed to detect changes then it will never update (client will not act on the changes that it has been able to process)

I would expect the methods above to return any etag in the response - even when the client has subsequently generated an error while trying to process the body of that response. This ensures that the client doesn't repeatedly download and process a message that it can't.

This seems to be caused by the HTTP client - which fails to return the response if it encounters an unmarshal error. So, neither of the two methods above are able to retrieve the etag from the response that hasn't been returned.
https://github.com/firebase/firebase-admin-go/blob/74c9bd5edece6044ba365a04c32c00074006dd7c/internal/http_client.go#L170

**Workaround**
There is a workaround - the client can use json.RawMessage to defer parsing the response. This ensures that GetIfChanged and GetWithETag will not encounter an unmarshalling error and will therefore always return the etag.
```
var v json.RawMessage
etag, err := ref.GetWithETag(context.Background(), &v)
if err != nil {
// This can only be an error in the request - for example, a connection failure. Handle appropriately.
}
data := make()
err = json.Unmarshal(v, &data)
if err != nil {
// This could be one of three errors: InvalidUnmarshalError , SyntaxError, or UnmarshalTypeError.
// InvalidUnmarshalError indicates that data is nil; SyntaxError indicates that Firebase has returned non-json data
// UnmarshalTypeError indicates at least one schema error in the data - but data object will be populated as best it can be
// Handle appropriately.
}
```

Contributor guide

Open the contributing guide

Research direction

Start in internal/http_client.go around line 170, then trace the response handling from db/ref.go around lines 93 and 130 for GetWithETag and GetIfChanged. Verify how json.Unmarshal errors affect access to the response etag and changed result; done means both methods preserve the response etag and correctly report change state when partial unmarshalling returns an error.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.