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

Remove ambiguity in GetUserRepositories SQL #393

Merged
merged 1 commit into from
Jan 14, 2017
Merged

Remove ambiguity in GetUserRepositories SQL #393

merged 1 commit into from
Jan 14, 2017

Conversation

btrepp
Copy link
Contributor

@btrepp btrepp commented Dec 15, 2016

This includes all columns of the repository table in the group by clause
Certain databases will attempt to infer the other columns eg.
https://dev.mysql.com/doc/refman/5.7/en/group-by-handling.html

As some databases supported by xorm don't have the ability to do this
we include every column in the group by clause, this should work the
same assumining repository.id is unique.

This pull request will fix issues viewing the organisation page with #383.

@lunny lunny added the type/enhancement An improvement of existing functionality label Dec 15, 2016
@lunny lunny added this to the 1.1.0 milestone Dec 15, 2016
@tboerger
Copy link
Member

The indentation looks far too much and beside that the quoting is not database agnostic.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 15, 2016
@btrepp
Copy link
Contributor Author

btrepp commented Dec 16, 2016

Sorry about the indentation, i'm not 100% what whitespace characters are used in these files,I just replicated the whitespace characters that existed in vim, which looks fine in vim, but seems odd here. I will fix this.

Are you happy with the approach taken?. I think I would prefer to improve it to use some sort of reflection on the Repository struct, but would like to know if the maintainers think this is a good strategy to fix this issue before I spend too much time on it.

@tboerger
Copy link
Member

I'm not really happy with this kind of query, but I can't remember real alternatives in mssql beside sub queries.

@btrepp
Copy link
Contributor Author

btrepp commented Dec 16, 2016

I'm not thrilled with it myself, but its what I'm running currently to have gitea usable. Nested selects/sub queries is how I would usually solve this, and would be database agnostic, but from what I can tell xorm doesn't really support them (which makes sense, it should be generating queries). So maybe the issue is upstream with xorm?.

Options I've thought to fix this are.

  1. Group/or select MAX() columns we only want, the motivation for all columns in the PR is that I don't know exactly how these data structures are used, so its safer to fully populate it
  2. Build the group by/select via reflection, I began working on this, but you would need to access xorms struct-column mapper to make sure you got the correct table naming (it appears gitea uses the snake_case mapper, but it would be better to automatically use whatever xorm is using).
  3. RAW SQL that is database agnostic, which wouldn't be too difficult... except that gitea is paging the results and MSSQL uses completely different literals for doing that.
  4. Do a nested select, but do it in two stages to make xorm happy, this could work, but would involve pulling out all of the users team_repos, much like how the users teams is pulled out. I would be concerned about the performance of doing this when a user is involved in many repositories/teams. It would render the performance gain of paging that query redundant.
  5. Deciding to abandon MSSQL and only supported databases that can work around more ambiguous SQL queries.

@tboerger
Copy link
Member

I'm totally fine with multi database support, and it's also good that you have skipped the reflection way.

I'm just thinking about two queries. One fetching only the unique ids, one to fetch the real repositories.

@btrepp
Copy link
Contributor Author

btrepp commented Dec 21, 2016

I've updated it to do in in two queries like you suggested. I'm not sure whether this function is becoming a bit mammoth now though.

@tboerger
Copy link
Member

@lunny suggestions?

@lunny
Copy link
Member

lunny commented Dec 21, 2016

I will give it L-G-T-M after I tested it. @tboerger

@tboerger
Copy link
Member

Got conflicts

Breaks the retrieval of repositories into two queries
This fetches the paged ids in one go, then the
actual repository information in a second query

Some databases do not support SELECT with *
when group by is used.
@btrepp btrepp closed this Jan 10, 2017
@btrepp btrepp deleted the orgsqlfix branch January 10, 2017 06:34
@btrepp btrepp restored the orgsqlfix branch January 11, 2017 00:40
@btrepp btrepp reopened this Jan 11, 2017
@lunny
Copy link
Member

lunny commented Jan 14, 2017

LGTM

@tboerger tboerger 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 14, 2017
@appleboy
Copy link
Member

LGTM

@tboerger tboerger 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 14, 2017
@lunny lunny merged commit 302fa42 into go-gitea:master Jan 14, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants