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

Remove GetByBean method because sometimes it's danger when query condition parameter is zero and also introduce new generic methods #28220

Merged
merged 19 commits into from Dec 7, 2023
Merged

Remove GetByBean method because sometimes it's danger when query condition parameter is zero and also introduce new generic methods #28220

merged 19 commits into from Dec 7, 2023

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 26, 2023 •
edited

The function GetByBean has an obvious defect that when the fields are empty values, it will be ignored. Then users will get a wrong result which is possibly used to make a security problem.

To avoid the possibility, this PR removed function GetByBean and all references.
And some new generic functions have been introduced to be used.

The recommand usage like below.

// if query an object according id
obj, err := db.GetByID[Object](ctx, id)
// query with other conditions
obj, err := db.Get[Object](ctx, builder.Eq{"a": a, "b":b})

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 26, 2023
@lunny lunny added this to the 1.21.2 milestone Nov 26, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 26, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 26, 2023
@lunny lunny changed the title Remove GetByBean method because sometimes it's danger when query condition parameter is zero WIP: Remove GetByBean method because sometimes it's danger when query condition parameter is zero Nov 26, 2023
@lunny lunny modified the milestones: 1.21.2, 1.22.0 Nov 26, 2023
@lunny lunny changed the title WIP: Remove GetByBean method because sometimes it's danger when query condition parameter is zero Remove GetByBean method because sometimes it's danger when query condition parameter is zero and also introduce new generic methods Nov 26, 2023
delvh
delvh reviewed Nov 26, 2023
View reviewed changes
models/db/list.go Outdated Show resolved Hide resolved
models/git/lfs.go Outdated Show resolved Hide resolved
models/db/context.go Show resolved Hide resolved
models/issues/label.go Outdated Show resolved Hide resolved
@delvh
Copy link
Member

delvh commented Nov 26, 2023

https://github.com/go-gitea/gitea/actions/runs/6995519996/job/19030466897#step:8:235

delvh
delvh approved these changes Dec 3, 2023
View reviewed changes
models/db/context.go Outdated Show resolved Hide resolved
models/db/context.go Outdated Show resolved Hide resolved
@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 Dec 3, 2023
wolfogre
wolfogre reviewed Dec 7, 2023
View reviewed changes
models/db/list.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 7, 2023
@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 7, 2023
@lunny lunny merged commit dd30d9d into go-gitea:main Dec 7, 2023
25 checks passed
@lunny lunny deleted the lunny/make_function_call_safer branch December 7, 2023 07:27
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 7, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 8, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
13beca8 
* giteaofficial/main:
  Improve text in Security settings (go-gitea#28393)
  Fix Docker meta action for releases (go-gitea#28232)
  Make gogit Repository.GetBranchNames consistent (go-gitea#28348)
  Remove GetByBean method because sometimes it's danger when query condition parameter is zero and also introduce new generic methods (go-gitea#28220)
  Include public repos in doer's dashboard for issue search (go-gitea#28304)
  Issue fixes for RSS feed improvements (go-gitea#28380)
  Fix margin in server signed signature verification view (go-gitea#28379)
  [skip ci] Updated translations via Crowdin
  Fix incorrect run order of action jobs (go-gitea#28367)
  Improve RSS feed icons (go-gitea#28368)
  Use `filepath` instead of `path` to create SQLite3 database file (go-gitea#28374)
  Fix incorrect default value of `[attachment].MAX_SIZE` (go-gitea#28373)
  Fix object does not exist error when checking citation file (go-gitea#28314)
@lunny lunny mentioned this pull request Dec 25, 2023
Merged
lunny added a commit that referenced this pull request Dec 27, 2023
@lunny
Fix session key conflict with database keyword (#28613)
4c29c75 
This is a regression from #28220 .
`builder.Cond` will not add `` ` `` automatically but xorm method
`Get/Find` adds `` ` ``.

This PR also adds tests to prevent the method from being implemented
incorrectly. The tests are added in `integrations` to test every
database.
@lunny lunny mentioned this pull request Dec 31, 2023
Merged
lunny added a commit that referenced this pull request Jan 15, 2024
@lunny @delvh
Move more functions to db.Find (#28419)
70c4aad 
Following #28220

This PR move more functions to use `db.Find`.

---------

Co-authored-by: delvh <dev.lh@web.de>
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @fuxiaohei
Remove GetByBean method because sometimes it's danger when query cond…
fa445f6 
…ition parameter is zero and also introduce new generic methods (go-gitea#28220)

The function `GetByBean` has an obvious defect that when the fields are
empty values, it will be ignored. Then users will get a wrong result
which is possibly used to make a security problem.

To avoid the possibility, this PR removed function `GetByBean` and all
references.
And some new generic functions have been introduced to be used.

The recommand usage like below.

```go
// if query an object according id
obj, err := db.GetByID[Object](ctx, id)
// query with other conditions
obj, err := db.Get[Object](ctx, builder.Eq{"a": a, "b":b})
```
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @fuxiaohei
Fix session key conflict with database keyword (go-gitea#28613)
f033c76 
This is a regression from go-gitea#28220 .
`builder.Cond` will not add `` ` `` automatically but xorm method
`Get/Find` adds `` ` ``.

This PR also adds tests to prevent the method from being implemented
incorrectly. The tests are added in `integrations` to test every
database.
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lunny @delvh @fuxiaohei
Move more functions to db.Find (go-gitea#28419)
23179ec 
Following go-gitea#28220

This PR move more functions to use `db.Find`.

---------

Co-authored-by: delvh <dev.lh@web.de>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @silverwind
Remove GetByBean method because sometimes it's danger when query cond…
44fee2e 
…ition parameter is zero and also introduce new generic methods (go-gitea#28220)

The function `GetByBean` has an obvious defect that when the fields are
empty values, it will be ignored. Then users will get a wrong result
which is possibly used to make a security problem.

To avoid the possibility, this PR removed function `GetByBean` and all
references.
And some new generic functions have been introduced to be used.

The recommand usage like below.

```go
// if query an object according id
obj, err := db.GetByID[Object](ctx, id)
// query with other conditions
obj, err := db.Get[Object](ctx, builder.Eq{"a": a, "b":b})
```
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @silverwind
Fix session key conflict with database keyword (go-gitea#28613)
d57f69d 
This is a regression from go-gitea#28220 .
`builder.Cond` will not add `` ` `` automatically but xorm method
`Get/Find` adds `` ` ``.

This PR also adds tests to prevent the method from being implemented
incorrectly. The tests are added in `integrations` to test every
database.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lunny @delvh @silverwind
Move more functions to db.Find (go-gitea#28419)
3396b9a 
Following go-gitea#28220

This PR move more functions to use `db.Find`.

---------

Co-authored-by: delvh <dev.lh@web.de>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wolfogre wolfogre wolfogre approved these changes

@delvh delvh delvh 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lunny @delvh @wolfogre @GiteaBot