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

[BugFix] remove nil inserts in models #11096

Merged
merged 12 commits into from
Apr 17, 2020
Merged

[BugFix] remove nil inserts in models #11096

merged 12 commits into from
Apr 17, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 16, 2020 •
edited
Loading

this blocks #9084

@6543 6543 mentioned this pull request Apr 16, 2020
Merged
@6543 6543 changed the title [BugFix] removenil inserts in models [BugFix] remove nil inserts in models Apr 16, 2020
@jolheiser jolheiser added this to the 1.12.0 milestone Apr 16, 2020
6543
6543 commented Apr 16, 2020
View reviewed changes
modules/migrations/gitea.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 16, 2020
zeripath
zeripath reviewed Apr 16, 2020
View reviewed changes
models/migrate.go Outdated Show resolved Hide resolved
models/migrate.go Outdated Show resolved Hide resolved
modules/migrations/gitea.go Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Apr 16, 2020

@zeripath done

@guillep2k
Copy link
Member

🤔 I must be missing something, here. Since xorm should ignore empty arrays on insert, what bug is this PR fixing? (I understand it might be faster doing the if on this side and skip the xorm call, however).

@6543
Copy link
Member Author

6543 commented Apr 17, 2020

@guillep2k could be a xorm bug but if I remove this cheks migration freeze or xorm throw error: "No element on slice when insert" this is due to xorm update to 1.1...

@6543
Copy link
Member Author

6543 commented Apr 17, 2020 •
edited
Loading

@lunny could explain this better - but it breaks migration at least combined with gitlab migration pull

@lunny
Copy link
Member

lunny commented Apr 17, 2020

Before xorm v1.0, empty slice when inserting will be ignored and returned successful, but now it will return an error. So users have to check the length of the slice itself.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 17, 2020
@6543
Copy link
Member Author

6543 commented Apr 17, 2020

@guillep2k / @zeripath would be nice if you can approve this (or tell me what I have to change)
since I need this pull :)

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 17, 2020
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Apr 17, 2020
@lafriks lafriks merged commit 12960b9 into go-gitea:master Apr 17, 2020
@lafriks lafriks deleted the fix-models-nil-inserts-refactor branch April 17, 2020 17:43
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@6543 @zeripath
[BugFix] remove nil inserts in models (go-gitea#11096)
ba9005a 
* Fix InsertReleases Nil Insert on Attachments

* FIX "No element on slice when insert" & smal refactor

* again

* impruve

* rm useles newline

* Apply suggestions from code review

Co-Authored-By: zeripath <[email protected]>

* process insert as a whole

Co-authored-by: zeripath <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath left review comments

@lafriks lafriks lafriks approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.
7 participants
@6543 @guillep2k @lunny @lafriks @zeripath @GiteaBot @jolheiser