-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CBG-3835 catch error in case there is a valid json but not a JSON object #6756
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns about the overhead of the json.Valid call. It also feels like these changes are too heavyweight when considering the original enhancement we're trying to make here (CBG-3332), particularly considering this would be a backport candidate.
log in the case where there is _sync xattr but the body has become non json
The code has now been reverted to the state before 2bde1ea with two changes:
Wrote tests for the expected behavior, which is to silently ignore documents with invalid _sync metadata, where it is valid JSON but not a valid JSON object. |
require.Equal(t, int64(0), db.DbStats.SharedBucketImport().ImportErrorCount.Value()) | ||
} | ||
|
||
func TestImportFeedNonJSONNewDoc(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is logging:
Found sync metadata, but unable to unmarshal for feed document "<ud>bookstand</ud>". Will not be imported. Error: readObjectStart: expect { or n, but found 1, error found in #1 byte of ...|1|..., bigger context ...|1|...
which is incorrect, because it didn't find sync metadata. I think we've decided we can't differentiate between this case and the ones above without additional overhead, so I think we need to change the error message to be more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the error messages and wrote them inline with the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor tweak for the debug logging, otherwise looks good.
Co-authored-by: Adam Fraser <adam.fraser@couchbase.com>
backport of CBG-3835 catch error in case there is a valid json but not a JSON object (#6756) * Remove logging for invalid JSON in sync attribute log in the case where there is _sync xattr but the body has become non json * Update log messages * Update db/import_listener.go Co-authored-by: Adam Fraser <adam.fraser@couchbase.com> --------- Co-authored-by: Adam Fraser <adam.fraser@couchbase.com>
* [CBG-3876] suppress warning for non json objects backport of CBG-3835 catch error in case there is a valid json but not a JSON object (#6756) * Remove logging for invalid JSON in sync attribute log in the case where there is _sync xattr but the body has become non json * Update log messages * Update db/import_listener.go Co-authored-by: Adam Fraser <adam.fraser@couchbase.com> --------- Co-authored-by: Adam Fraser <adam.fraser@couchbase.com> * Change test to work without multi-xattr write APIs --------- Co-authored-by: Adam Fraser <adam.fraser@couchbase.com>
The code has now been reverted to the state before 2bde1ea with two changes:
log debug for event.DataType == base.MemcachedDataTypeRaw, Info logging in 3.1.3/3.1.4 but no logging before
increment ImportErrorCount in the case of valid _sync xattr and valid json but not a json object
Wrote tests for the expected behavior, which is to silently ignore documents with invalid _sync metadata, where it is valid JSON but not a valid JSON object.
Pre-review checklist
fmt.Print
,log.Print
, ...)base.UD(docID)
,base.MD(dbName)
)docs/api
Integration Tests
GSI=true,xattrs=true
https://jenkins.sgwdev.com/job/SyncGateway-Integration/2380/