Skip to content
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.

Sign up for GitHub

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

refactor: Use fastjson to parse mutation data string #772

Merged
merged 10 commits into from
Sep 13, 2022
Merged

refactor: Use fastjson to parse mutation data string #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2305106
change to fastjson for field validation
fredcarle Sep 2, 2022
fec052c
merge convertNillableArray with getArray
fredcarle Sep 2, 2022
aa2f875
change to zeroValue as the determinator of nillable array
fredcarle Sep 2, 2022
5c2495b
remove unnecessary declarations
fredcarle Sep 2, 2022
fe6f0be
apply feedback
fredcarle Sep 2, 2022
1378437
apply feedback
fredcarle Sep 2, 2022
bb15112
add unit test
fredcarle Sep 8, 2022
a911ead
remove linting from unused patch related functions
fredcarle Sep 8, 2022
d6502df
move test to integration
fredcarle Sep 12, 2022
f0275c5
implement feedback
fredcarle Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions client/collection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,26 +83,26 @@ type Collection interface {
//
// Returns an ErrInvalidUpdateTarget error if the target type is not supported.
// Returns an ErrInvalidUpdater error if the updater type is not supported.
UpdateWith(ctx context.Context, target interface{}, updater interface{}) (*UpdateResult, error)
UpdateWith(ctx context.Context, target interface{}, updater string) (*UpdateResult, error)
// UpdateWithFilter updates using a filter to target documents for update.
//
// The provided updater must be a string Patch, string Merge Patch, a parsed Patch, or parsed Merge Patch
// else an ErrInvalidUpdater will be returned.
UpdateWithFilter(ctx context.Context, filter interface{}, updater interface{}) (*UpdateResult, error)
UpdateWithFilter(ctx context.Context, filter interface{}, updater string) (*UpdateResult, error)
// UpdateWithKey updates using a DocKey to target a single document for update.
//
// The provided updater must be a string Patch, string Merge Patch, a parsed Patch, or parsed Merge Patch
// else an ErrInvalidUpdater will be returned.
//
// Returns an ErrDocumentNotFound if a document matching the given DocKey is not found.
UpdateWithKey(ctx context.Context, key DocKey, updater interface{}) (*UpdateResult, error)
UpdateWithKey(ctx context.Context, key DocKey, updater string) (*UpdateResult, error)
// UpdateWithKeys updates documents matching the given DocKeys.
//
// The provided updater must be a string Patch, string Merge Patch, a parsed Patch, or parsed Merge Patch
// else an ErrInvalidUpdater will be returned.
//
// Returns an ErrDocumentNotFound if a document is not found for any given DocKey.
UpdateWithKeys(context.Context, []DocKey, interface{}) (*UpdateResult, error)
UpdateWithKeys(context.Context, []DocKey, string) (*UpdateResult, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for this type change - this always confused me and I thought there was more types that this could handle but the diff suggests it was just the two. Do check in with John though if you haven't already to make sure we are not losing anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks :)

As far as I could tell the value passed was always a string but yes I will be checking with John.

Copy link
Member

Choose a reason for hiding this comment

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

The idea was to be able to support native Go types map[string]interface{} If using the programatic approach, but at the moment, its only used/tested from the POV of the query system, which only handles strings.

I would nice to still support this, but its prob lower priority, and supporting native types gets in the way of using the fastjson approach outlined in this PR anyway. So its fine for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have a look at the last commit for a potential solution to keep Go maps as a possible updater


// DeleteWith deletes a target document.
//
Expand Down
Loading