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.

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

feat: delete record endpoint #3337

Merged
merged 164 commits into from
Aug 2, 2023
Merged

Conversation

gabrielmbmb
Copy link
Member

@gabrielmbmb gabrielmbmb commented Jul 4, 2023

Description

This PR adds one new endpoint for deleting records:

  • DELETE /api/v1/records/{record_id}: for removing a specific record given its ID.

It's been needed to add a new method delete_records to the SearchEngine that will remove the records also from the index created for the dataset in Elastic Search.

Closes #3310

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested

Manually in a local environment and I've added unit tests.

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

I think makes sense to return in the future a 422 when bulk_delete with non-existing records.

@alvarobartt
Copy link
Member

I think makes sense to return in the future a 422 when bulk_delete with non-existing records.

Shouldn't we return 404 in that case? As we want to state that the introduced record IDs were not found, right?

@frascuchon frascuchon removed this from the v1.13.0 milestone Jul 17, 2023
@gabrielmbmb gabrielmbmb force-pushed the feature/delete-records-endpoints branch from ad90ded to d73729f Compare August 1, 2023 11:41
@gabrielmbmb gabrielmbmb changed the title feat: delete records endpoints feat: delete record endpoint Aug 1, 2023
@gabrielmbmb gabrielmbmb force-pushed the feature/delete-records-endpoints branch from 54a17d4 to 717a776 Compare August 1, 2023 11:43
@frascuchon frascuchon self-requested a review August 1, 2023 12:06
Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

💯

@gabrielmbmb gabrielmbmb merged commit 8d81d05 into develop Aug 2, 2023
14 checks passed
@gabrielmbmb gabrielmbmb deleted the feature/delete-records-endpoints branch August 2, 2023 07:23
keithCuniah pushed a commit that referenced this pull request Aug 3, 2023
# Description

This PR adds one new endpoint for deleting records:

- `DELETE /api/v1/records/{record_id}`: for removing a specific record
given its ID.

It's been needed to add a new method `delete_records` to the
`SearchEngine` that will remove the records also from the index created
for the dataset in Elastic Search.

Closes #3310

**Type of change**

- [x] New feature (non-breaking change which adds functionality)


**How Has This Been Tested**

Manually in a local environment and I've added unit tests.

**Checklist**

- [ ] I added relevant documentation
- [x] follows the style guidelines of this project
- [x] I did a self-review of my code
- [ ] I made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [x] I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: Francisco Aranda <francis@argilla.io>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints type: enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add API v1 endpoints to remove records
3 participants