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 non-ASCII search on database #18437

Merged
merged 7 commits into from
Feb 1, 2022
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Jan 28, 2022

Gusted added 2 commits January 28, 2022 02:14
- Use `ToASCIIUpper` for SQLite database on issues search, this because
`UPPER(x)` on SQLite only transforms ASCII letters.
- Resolves go-gitea#18429
@Gusted Gusted added type/enhancement An improvement of existing functionality type/bug labels Jan 28, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jan 28, 2022
)

// ToASCIIUpper returns s with all ASCII letters mapped to their upper case.
func ToASCIIUpper(s string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Nothing in golang's stdlib for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, the closes things is ToUpperSpecial, but the struct that is passed is more or less magic to me and has a unexported field. This function is a modification from ToUpper with the same test cases.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 28, 2022
@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 Jan 29, 2022
@wxiaoguang
Copy link
Contributor

It's fine to me and I have no objection.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM. Also hope more people to take a look at this solution. We can keep this PR open for a few more days. If nothing more to do, then we can merge it.

@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 Jan 29, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Jan 29, 2022

LGTM. Also hope more people to take a look at this solution. We can keep this PR open for a few more days. If nothing more to do, then we can merge it.

Yeah, this solution is "hacky" to be more inline with the usage of Sqlite... A problem is that, this isn't the only usage of UPPER(xxx), just to happen to be the one noticed by the issue. Best-case scenario is that we can define a collation trough XORM which will ensure that accents like these are properly covered.

@wxiaoguang wxiaoguang merged commit bb5f859 into go-gitea:main Feb 1, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 2, 2022
* giteaofficial/main: (37 commits)
  Collaborator trust model should trust collaborators (go-gitea#18539)
  Detect conflicts with 3way merge (go-gitea#18536)
  [skip ci] Updated translations via Crowdin
  Update 1.16.0 changelog to set go-gitea#17846 as breaking (go-gitea#18533)
  In docker rootless use $GITEA_APP_INI if provided (go-gitea#18524)
  revert to node14 for snapcraft
  Add `GetUserTeams` (go-gitea#18499)
  Fix review excerpt (go-gitea#18502)
  Update JS dependencies, fix lint (go-gitea#18389)
  add test coverage for original author conversion during migrations (go-gitea#18506)
  add gitea-fmt back (go-gitea#18526)
  Fix non-ASCII search on database  (go-gitea#18437)
  Use "read" value for General Access (go-gitea#18496)
  Fix for AvatarURL database type (go-gitea#18487)
  Remove go 1.15 support (go-gitea#18511)
  [skip ci] Updated translations via Crowdin
  Use `ImagedProvider` for gplus oauth2 provider (go-gitea#18504)
  build with node16 in snap (go-gitea#18508)
  point to s3 endpoint directly (go-gitea#18497)
  Fix OAuth Source Edit Page (go-gitea#18495)
  ...
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Use `ToASCIIUpper` for SQLite database on issues search, this because `UPPER(x)` on SQLite only transforms ASCII letters. Resolves go-gitea#18429
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
@Gusted Gusted deleted the fix-sqlite-search branch February 9, 2023 13:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search to accents not working with sqlite
5 participants