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] Account Linking UpdateMigrationsByType #31428

Merged
merged 8 commits into from
Jun 20, 2024

Conversation

Sumit189
Copy link
Contributor

@Sumit189 Sumit189 commented Jun 19, 2024

Changes

  • Added parsing check externalUserId in the UpdateMigrationsByType function. If parsing fails, subsequent function calls are skipped.

Related to: #31427

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 19, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 19, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/migrations labels Jun 19, 2024
@wxiaoguang
Copy link
Contributor

Sorry but IMO it is not likely a complete fix.

When using Google OAuth, the UserID returned can be quite large, such as 1108672xxx5299468800.

IIRC, when using OAuth, the "UserID" can be any string, it doesn't need to be a number.

So the root problem is that OAuth UserID shouldn't be used as original_author_id directly.

@wxiaoguang
Copy link
Contributor

I would suggest to do this, either:

  • simple fix: add a ParseInt64 check in UpdateMigrationsByType, if the parsing fails, do not call these functions, and leave a comment.
  • complete fix: change "original_author_id" type to string and refactor all related code.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 20, 2024

Thank you for trying the "complete" fix, but it would be very complex and difficult.

The problem is that "original_author_id" is indexed, if it is a varchar, then original_author_id=0 (in existing SQLs) won't be able to use the index, then all related queries become full-table scan, which will introduce performance regression. That's what I meant "refactor all related code" above.

Hmm, I see the related code, will review again.


To fix the problem quickly, I think the "simple fix" could be good enough, only a few lines.

@Sumit189
Copy link
Contributor Author

Sumit189 commented Jun 20, 2024

Thank you for trying the "complete" fix, but it would be very complex and difficult.

The problem is that "original_author_id" is indexed, if it is a varchar, then original_author_id=0 (in existing SQLs) won't be able to use the index, then all related queries become full-table scan, which will introduce performance regression. That's what I meant "refactor all related code" above.

Hmm, I see the related code, will review again.

To fix the problem quickly, I think the "simple fix" could be good enough, only a few lines.

I had tried adding ParseInt64 check but it didn't solve the purpose.

need to make changes to a few more places will push after testing those

@wxiaoguang
Copy link
Contributor

need to make changes to a few more places will push after testing those

I checkout the code, and I can see that there are still a lot of "OriginalAuthorID int64" in code ..... it is really a hard task ....

To make it consistency, we should also make the default value to empty string '' but not '0', more work ....


I had tried adding ParseInt64 check but it didn't solve the purpose.

Why it doesn't resolve?

I guess this should work?

// UpdateMigrationsByType updates all migrated repositories' posterid from gitServiceType to replace originalAuthorID to posterID
func UpdateMigrationsByType(ctx context.Context, tp structs.GitServiceType, externalUserID string, userID int64) error {
	if _, err := strconv.ParseInt(externalUserID, 10, 64); err != nil {
		return nil
	}

@Sumit189
Copy link
Contributor Author

Sumit189 commented Jun 20, 2024

need to make changes to a few more places will push after testing those

I checkout the code, and I can see that there are still a lot of "OriginalAuthorID int64" in code ..... it is really a hard task ....

To make it consistency, we should also make the default value to empty string '' but not '0', more work ....

I had tried adding ParseInt64 check but it didn't solve the purpose.

Why it doesn't resolve?

I guess this should work?

// UpdateMigrationsByType updates all migrated repositories' posterid from gitServiceType to replace originalAuthorID to posterID
func UpdateMigrationsByType(ctx context.Context, tp structs.GitServiceType, externalUserID string, userID int64) error {
	if _, err := strconv.ParseInt(externalUserID, 10, 64); err != nil {
		return nil
	}

Yes this should work, but it will prevent update operations from being performed, this is also an issue right

will try to update all relevant code for utilizing string

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 20, 2024

Yes this should work, but it will prevent update operations from being performed, this is also an issue right

We could just leave a comment: // for non-numeric strings, no need to update, I think it is right to ignore these non-numeric external IDs.

will try to update all relevant code for utilizing string

It would take a lot of time, if you really would like to try (and thank you for the brave!), the migration should refer to something like v286.go and have some tests because there are so many databases and they behave differently.

@wxiaoguang
Copy link
Contributor

ps: (the last suggestion for the "choice" 😁 ) I would still suggest to have a quick/simple fix first since it is right to ignore non-numeric external IDs for the "update migration type" .... the complete fix indeed seems good but it is really difficult to implement&review ..... if it introduces regressions then it would need more time to follow up ....

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2024
@Sumit189
Copy link
Contributor Author

ps: (the last suggestion for the "choice" 😁 ) I would still suggest to have a quick/simple fix first since it is right to ignore non-numeric external IDs for the "update migration type" .... the complete fix indeed seems good but it is really difficult to implement&review ..... if it introduces regressions then it would need more time to follow up ....

lets go with simple fix for now, have reverted other code

@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 Jun 20, 2024
@wxiaoguang wxiaoguang added type/bug backport/v1.22 This PR should be backported to Gitea 1.22 labels Jun 20, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) June 20, 2024 04:04
@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 Jun 20, 2024
@wxiaoguang wxiaoguang merged commit 17b3a38 into go-gitea:main Jun 20, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 20, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jun 20, 2024
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Jun 20, 2024
wxiaoguang pushed a commit that referenced this pull request Jun 20, 2024
Backport #31428 by Sumit189

Co-authored-by: Sumit <sumit.18.paul@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 21, 2024
* giteaofficial/main:
  Fix the link for .git-blame-ignore-revs bypass (go-gitea#31432)
  Bump htmx to 2.0.0 (go-gitea#31413)
  Fix the wrong line number in the diff view page when expanded twice. (go-gitea#31431)
  Fix labels and projects menu overflow on issue page (go-gitea#31435)
  [Fix] Account Linking UpdateMigrationsByType  (go-gitea#31428)
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 21, 2024
* origin/main: (21 commits)
  Fix deprecated Dockerfile ENV format (go-gitea#31450)
  README Badge maintenance (go-gitea#31441)
  Improve markdown textarea for indentation and lists (go-gitea#31406)
  Split common-global.js into separate files (go-gitea#31438)
  Fix the link for .git-blame-ignore-revs bypass (go-gitea#31432)
  Bump htmx to 2.0.0 (go-gitea#31413)
  Fix the wrong line number in the diff view page when expanded twice. (go-gitea#31431)
  Fix labels and projects menu overflow on issue page (go-gitea#31435)
  [Fix] Account Linking UpdateMigrationsByType  (go-gitea#31428)
  Fix markdown math brackets render problem (go-gitea#31420)
  Reduce `air` verbosity (go-gitea#31417)
  Fix new issue/pr avatar (go-gitea#31419)
  Increase max length of org team names from 30 to 255 characters (go-gitea#31410)
  [skip ci] Updated translations via Crowdin
  Refactor names (go-gitea#31405)
  Update JS dependencies, remove `eslint-plugin-jquery` (go-gitea#31402)
  Switch to upstream of `gorilla/feeds` (go-gitea#31400)
  Fix rendered wiki page link (go-gitea#31398)
  Refactor repo unit "disabled" check (go-gitea#31389)
  Refactor route path normalization (go-gitea#31381)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Account Linking] Error 500 Post Account Linking Due To original_author_id BigInt Size Limitation
4 participants