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

Jump to bottom

feat: Add ability to delete multiple documents using filter. #206

Merged
merged 4 commits into from
Feb 22, 2022
Merged

feat: Add ability to delete multiple documents using filter. #206

merged 4 commits into from
Feb 22, 2022

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Feb 11, 2022 •
edited
Loading

Resolves #166
Resolves #167

Adds the ability to delete documents that match the given filter.

For example:

mutation {
    delete_user(filter: {name: {_eq: "Shahzad"}}) {
        DeletedKey: _key
    }
}

Gives this on successful deletion of all documents with name = Shahzad:

{
  "data": [
    {
      "DeletedKey": "bae-6a6482a8-24e1-5c73-a237-ca569e41507d"
    },
    {
      "DeletedKey": "bae-de1b3170-b0fa-5ae9-94d6-ae44ad560b24"
    }
  ]
}

@shahzadlone shahzadlone self-assigned this Feb 11, 2022
@shahzadlone shahzadlone added the feature New feature or request label Feb 11, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.2.1 milestone Feb 11, 2022
This was linked to issues Feb 11, 2022
Delete Documents that match the filter #166
Closed
Test Documents Deletion with matching filter #167
Closed
@codecov
Copy link

codecov bot commented Feb 11, 2022 •
edited
Loading

Codecov Report

Merging #206 (f651d9d) into develop (270fba2) will increase coverage by 0.02%.
The diff coverage is 65.59%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #206      +/-   ##
===========================================
+ Coverage    57.90%   57.93%   +0.02%     
===========================================
  Files           99       99              
  Lines         9571     9625      +54     
===========================================
+ Hits          5542     5576      +34     
- Misses        3413     3425      +12     
- Partials       616      624       +8
Impacted Files Coverage Δ
db/collection_update.go 42.00% <57.14%> (+0.13%) ⬆️
db/collection_delete.go 56.73% <64.47%> (+0.61%) ⬆️
query/graphql/planner/delete.go 75.00% <80.00%> (+1.38%) ⬆️

@shahzadlone shahzadlone changed the title feat: Implement delete document using filter. feat: Add ability to delete multiple documents using filter. Feb 15, 2022
@shahzadlone shahzadlone force-pushed the lone/feat/delete-documents-with-filter branch 2 times, most recently from 672ccd4 to 2ad0ec7 Compare February 15, 2022 10:36
@shahzadlone shahzadlone marked this pull request as ready for review February 15, 2022 10:38
@AndrewSisley
Copy link
Contributor

AndrewSisley commented Feb 15, 2022 •
edited
Loading 

{
  "data": [
    {
      "DeletedKey": "bae-6a6482a8-24e1-5c73-a237-ca569e41507d"
    },
  ]
}

I don't know what was decided before, but _key and/or DocKey would seem more consistent than DeletedKey here?

EDIT: You aliases this in the example - my bad, sorry :)

AndrewSisley
AndrewSisley approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks good to me, couple of minor points/questions and a bigger one that can perhaps be done in a new ticket now (given that it is RE older delete functionality)

db/collection_delete.go Outdated Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Outdated Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Show resolved Hide resolved
jsimnz
jsimnz requested changes Feb 15, 2022
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

A few minor things to change in addition to andy's comments

db/collection_delete.go Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Show resolved Hide resolved
db/tests/mutation/simple/delete/with_filter_test.go Show resolved Hide resolved
query/graphql/planner/delete.go Outdated Show resolved Hide resolved
@jsimnz jsimnz mentioned this pull request Feb 15, 2022
Closed
@shahzadlone shahzadlone force-pushed the lone/feat/delete-documents-with-filter branch from 5074ae9 to 2dd720d Compare February 18, 2022 21:07
jsimnz
jsimnz approved these changes Feb 20, 2022
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Minor comment on the named return values, but approving.

LGTM

db/collection_update.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone merged commit ffd2236 into develop Feb 22, 2022
@shahzadlone shahzadlone deleted the lone/feat/delete-documents-with-filter branch February 22, 2022 17:53
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@shahzadlone
feat: Add ability to delete multiple documents using filter. (sourcen…
017ac25 
…etwork#206)

ISSUES:
Resolves sourcenetwork#166
Resolves sourcenetwork#167

DESCRIPTION:
Adds the ability to delete documents that match the given filter.
For example:
```
mutation {
    delete_user(filter: {name: {_eq: "Shahzad"}}) {
        DeletedKey: _key
    }
}
```
Gives this on successful deletion of all documents with name = `Shahzad`:
```
{
  "data": [
    {
      "DeletedKey": "bae-6a6482a8-24e1-5c73-a237-ca569e41507d"
    },
    {
      "DeletedKey": "bae-de1b3170-b0fa-5ae9-94d6-ae44ad560b24"
    }
  ]
}
```

COMMITS:
* feat: Add deletion of documents that match a filter
* test: Add tests for deletion of documents with a filter
* Adhere to code review.
* Only log on `query.Close()`, do not overwrite return values using named
returns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewSisley AndrewSisley AndrewSisley approved these changes

@jsimnz jsimnz jsimnz approved these changes

Assignees

@shahzadlone shahzadlone

Labels
feature New feature or request
Projects
None yet
Milestone
DefraDB v0.2.1
Development

Successfully merging this pull request may close these issues.

Test Documents Deletion with matching filter Delete Documents that match the filter
3 participants
@shahzadlone @AndrewSisley @jsimnz