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

fix: add missing database commits after begin_nested #3567

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

gabrielmbmb
Copy link
Member

@gabrielmbmb gabrielmbmb commented Aug 16, 2023

Description

When using PostgreSQL, the changes made after a begin_nested where not being written to the database because missing commit. This didn't happen when using SQLite3, which was writing the data even without executing a commit.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

I've tested in a local development environment with PostgreSQL all the endpoints that were using begin_nested:

  • Publish dataset
  • Delete dataset
  • Delete record
  • Create records
  • Create response
  • Update response
  • Delete response

Checklist

  • I followed the style guidelines of this project
  • I did a self-review of my code
  • 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/)

@gabrielmbmb gabrielmbmb self-assigned this Aug 16, 2023
@gabrielmbmb gabrielmbmb added type: bug Indicates an unexpected problem or unintended behavior area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints labels Aug 16, 2023
@gabrielmbmb gabrielmbmb marked this pull request as ready for review August 16, 2023 14:08
@gabrielmbmb gabrielmbmb changed the title fix: add missing commits fix: add missing database commits after begin_nested Aug 16, 2023
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07% 🎉

Comparison is base (c536982) 90.41% compared to head (ae5a64c) 90.49%.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           releases/1.14.1    #3567      +/-   ##
===================================================
+ Coverage            90.41%   90.49%   +0.07%     
===================================================
  Files                  255      255              
  Lines                13535    13543       +8     
===================================================
+ Hits                 12238    12256      +18     
+ Misses                1297     1287      -10     
Files Changed Coverage Δ
src/argilla/server/contexts/accounts.py 97.18% <100.00%> (+0.04%) ⬆️
src/argilla/server/contexts/datasets.py 99.17% <100.00%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gabrielmbmb gabrielmbmb merged commit 0b0b4f5 into releases/1.14.1 Aug 16, 2023
@gabrielmbmb gabrielmbmb deleted the fix/begin-nested-commit branch August 16, 2023 14:43
artikandri pushed a commit to CLARIN-PL/argilla that referenced this pull request Oct 30, 2023
…3567)

# Description

When using PostgreSQL, the changes made after a `begin_nested` where not
being written to the database because missing `commit`. This didn't
happen when using SQLite3, which was writing the data even without
executing a `commit`.

**Type of change**

- [x] Bug fix (non-breaking change which fixes an issue)

**How Has This Been Tested**

I've tested in a local development environment with PostgreSQL all the
endpoints that were using `begin_nested`:

- [x] Publish dataset
- [x] Delete dataset
- [x] Delete record
- [x] Create records
- [x] Create response
- [x] Update response
- [x] Delete response

**Checklist**

- [x] I followed the style guidelines of this project
- [x] I did a self-review of my code
- [x] 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](https://tally.so/r/n9XrxK)
(see text above)
- [x] I have added relevant notes to the `CHANGELOG.md` file (See
https://keepachangelog.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: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant