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

Prevent NPE in CSV diff rendering when column removed #17018

Merged
merged 22 commits into from Oct 20, 2021

Conversation

richmahn
Copy link
Contributor

@richmahn richmahn commented Sep 11, 2021

Fixes #16837 if a column is deleted.

We were clobbering the columns that were added by looping through the aline (base) and then when bline (head) was looped through, it clobbered what was in the "cells" array that is show in the diff, and then left a nil cell because nothing was shifted.

This fix properly shifts the cells, and properly puts the b cell either at its location or after, according to what the aline placed in the cells.

This includes test, adding a new test function since adding/removing cells works best with three columns, not two, which results in 4 columns of the resulting cells because it has a deleted column and an added column. If you try this locally, you can try those cases and others, such as adding a column.

There was no need to do anything special for the rows when aline == 0 || bline == 0 so that was removed. This allows the same code to be used for removed or added lines, with the bcell text always being the RightCell, acell text being the LeftCell.

I still added the patch @zeripath gave at #16837 (comment) so that just in case for some reason a cell is nil (which shouldn't happen now) it doesn't throw a 500 error, so the user can at least view the raw diff.

Also fixes in the view.go file how if a CSV file is empty (either created empty or if you edit it and remove all contents) it throws a huge 500 error when you then save it (when you view the file). Since we allow creating, saving and pushing empty files, we shouldn't throw an error on an empty CSV file, but just show its empty contents. This doesn't happen if it is a Markdown file or other type of file that is empty.
EDIT: Now handled in the markup/csv renderer code

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 11, 2021
@richmahn
Copy link
Contributor Author

I see integration isn't passing, and I also found this case causes and index out of bounds error. Will fix.

BASE:

col1,col2,col3
a,b,c

HEAD:

col3,col4,col5
c,d,e

@richmahn
Copy link
Contributor Author

@silverwind @zeripath Ok, if you could play with this reworking of all things CSV Diff locally (try to break with all sorts of CSV diffs) and review, appreciate it!

@codecov-commenter

This comment has been minimized.

routers/web/repo/view.go Outdated Show resolved Hide resolved
@lunny lunny added this to the 1.16.0 milestone Sep 12, 2021
@richmahn richmahn force-pushed the richmahn-16837-csv-column-diff-error branch from 66e8960 to 9f70afe Compare September 12, 2021 23:49
@richmahn
Copy link
Contributor Author

Ok, fixed initial commit with EOF 500 error.

@zeripath zeripath changed the title Fixes #16837 - CSV column diff error when column removed Prevent NPE in CSV diff rendering when column removed Sep 14, 2021
modules/markup/csv/csv.go Outdated Show resolved Hide resolved
modules/markup/csv/csv.go Outdated Show resolved Hide resolved
modules/csv/csv.go Outdated Show resolved Hide resolved
services/gitdiff/csv.go Outdated Show resolved Hide resolved
services/gitdiff/csv.go Outdated Show resolved Hide resolved
services/gitdiff/csv.go Outdated Show resolved Hide resolved
services/gitdiff/csv.go Outdated Show resolved Hide resolved
@richmahn
Copy link
Contributor Author

Just a note, still working on this, have made great progress in making sure it is done right. Even adding in it noting when a column has been moved so that is brought to the attention of the person reviewing the PR.

@zeripath zeripath mentioned this pull request Sep 19, 2021
@richmahn
Copy link
Contributor Author

richmahn commented Oct 19, 2021

Finally got a chance to work on this after dealing with COVID and lots of other work. Hope this gets looked at. Please feel free to ask question but hopefully the comments make sense. Takes a bit of work to nicely show a CSV diff. As already stated, now also shows moved columns but accounts for column additions and deletions (that is, if columns before a column were added or deleted, it doesn't flag the existing column as moved).

Example diff:

image

So in the above, phone and email got moved to the end, other columns added and deleted.

You can see this CSV on try.gitea.io but of course the diff throws a 500 error there, hence this fix:

Orig: https://try.gitea.io/richmahntest/test/src/commit/4d2ec906dafbb5082ed1aa0a3f50107139c42711/names.csv
Changes: https://try.gitea.io/richmahntest/test/src/branch/master/names.csv

@zeripath @silverwind @KN4CK3R

@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 Oct 20, 2021
@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 Oct 20, 2021
@zeripath
Copy link
Contributor

make lgtm work

@zeripath zeripath merged commit 98f7013 into go-gitea:main Oct 20, 2021
@zeripath
Copy link
Contributor

Please send backport

@richmahn
Copy link
Contributor Author

Making backport!

richmahn added a commit to richmahn/gitea that referenced this pull request Oct 20, 2021
6543 pushed a commit that referenced this pull request Oct 20, 2021
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 21, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 21, 2021
* SECURITY
  * Upgrade Bluemonday to v1.0.16 (go-gitea#17372) (go-gitea#17374)
  * Ensure correct SSH permissions check for private and restricted users (go-gitea#17370) (go-gitea#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (go-gitea#17018) (go-gitea#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (go-gitea#17281) (go-gitea#17376)
  * Don't panic if we fail to parse U2FRegistration data (go-gitea#17304) (go-gitea#17371)
  * Ensure popup text is aligned left (backport for 1.15) (go-gitea#17343)
  * Ensure that git daemon export ok is created for mirrors (go-gitea#17243) (go-gitea#17306)
  * Disable core.protectNTFS (go-gitea#17300) (go-gitea#17302)
  * Use pointer for wrappedConn methods (go-gitea#17295) (go-gitea#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (go-gitea#17292)
  * Handle duplicate keys on GPG key ring (go-gitea#17242) (go-gitea#17284)
  * Fix SVG side by side comparison link (go-gitea#17375) (go-gitea#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Oct 21, 2021
6543 pushed a commit that referenced this pull request Oct 21, 2021
* SECURITY
  * Upgrade Bluemonday to v1.0.16 (#17372) (#17374)
  * Ensure correct SSH permissions check for private and restricted users (#17370) (#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (#17018) (#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (#17281) (#17376)
  * Don't panic if we fail to parse U2FRegistration data (#17304) (#17371)
  * Ensure popup text is aligned left (backport for 1.15) (#17343)
  * Ensure that git daemon export ok is created for mirrors (#17243) (#17306)
  * Disable core.protectNTFS (#17300) (#17302)
  * Use pointer for wrappedConn methods (#17295) (#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (#17292)
  * Handle duplicate keys on GPG key ring (#17242) (#17284)
  * Fix SVG side by side comparison link (#17375) (#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 22, 2021
Frontport go-gitea#17392

* SECURITY
  * Upgrade Bluemonday to v1.0.16 (go-gitea#17372) (go-gitea#17374)
  * Ensure correct SSH permissions check for private and restricted users (go-gitea#17370) (go-gitea#17373)
* BUGFIXES
  * Prevent NPE in CSV diff rendering when column removed (go-gitea#17018) (go-gitea#17377)
  * Offer rsa-sha2-512 and rsa-sha2-256 algorithms in internal SSH (go-gitea#17281) (go-gitea#17376)
  * Don't panic if we fail to parse U2FRegistration data (go-gitea#17304) (go-gitea#17371)
  * Ensure popup text is aligned left (backport for 1.15) (go-gitea#17343)
  * Ensure that git daemon export ok is created for mirrors (go-gitea#17243) (go-gitea#17306)
  * Disable core.protectNTFS (go-gitea#17300) (go-gitea#17302)
  * Use pointer for wrappedConn methods (go-gitea#17295) (go-gitea#17296)
  * AutoRegistration is supposed to be working with disabled registration (backport) (go-gitea#17292)
  * Handle duplicate keys on GPG key ring (go-gitea#17242) (go-gitea#17284)
  * Fix SVG side by side comparison link (go-gitea#17375) (go-gitea#17391)

Signed-off-by: Andrew Thornton <art27@cantab.net>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Fixes go-gitea#16837 if a column is deleted.

We were clobbering the columns that were added by looping through the aline (base) and then when bline (head) was looped through, it clobbered what was in the "cells" array that is show in the diff, and then left a nil cell because nothing was shifted.

This fix properly shifts the cells, and properly puts the b cell either at its location or after, according to what the aline placed in the cells.

This includes test, adding a new test function since adding/removing cells works best with three columns, not two, which results in 4 columns of the resulting cells because it has a deleted column and an added column. If you try this locally, you can try those cases and others, such as adding a column.

There was no need to do anything special for the rows when `aline == 0 || bline == 0` so that was removed. This allows the same code to be used for removed or added lines, with the bcell text always being the RightCell, acell text being the LeftCell.

I still added the patch zeripath gave at go-gitea#16837 (comment) so that just in case for some reason a cell is nil (which shouldn't happen now) it doesn't throw a 500 error, so the user can at least view the raw diff.

Also fixes in the [view.go](https://github.com/go-gitea/gitea/pull/17018/files#diff-43a7f4747c7ba8bff888c9be11affaafd595fd55d27f3333840eb19df9fad393L521) file how if a CSV file is empty (either created empty or if you edit it and remove all contents) it throws a huge 500 error when you then save it (when you view the file). Since we allow creating, saving and pushing empty files, we shouldn't throw an error on an empty CSV file, but just show its empty contents. This doesn't happen if it is a Markdown file or other type of file that is empty.
EDIT: Now handled in the markup/csv renderer code
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

500 on CSV diff: runtime error: index out of range
8 participants