PR#343: Avoid to upload a file with different checksum - rpkg

rpkg

#343 Avoid to upload a file with different checksum

Merged 5 years ago by cqi. Opened 5 years ago by cqi.

cqi/rpkg fix-upload-file-with-different-checksum into master

Avoid to upload a file with different checksum

Chenxiong Qi • 5 years ago

4bd1e75

pyrpkg/sources.py

file modified

+9 -1

		`@@ -19,7 +19,7 @@`
		`import os`
		`import re`

		`- from .errors import HashtypeMixingError, MalformedLineError`
		`+ from .errors import rpkgError, HashtypeMixingError, MalformedLineError`


		`LINE_PATTERN = re.compile(`
		`@@ -80,6 +80,14 @@`
		`if entry == e:`
		`return`

		`+ if e.file == entry.file and e.hash != entry.hash:`
		`+ raise rpkgError(`
		`+ 'Uploading file {0} which has different checksum.\n'`
		`+ 'If this is the only file in sources, please use '`
		`+ 'new-sources.\n'`
		`+ 'If mutilple files are in sources, please remove this '`
		`+ 'file from sources manually.'.format(e.file))`
		`+`
		`self.entries.append(entry)`

		`def write(self):`

tests/test_cli.py

file modified

+20

		`@@ -1074,6 +1074,7 @@`


		`class TestUpload(LookasideCacheMock, CliTestCase):`
		`+ """Test command upload"""`

		`def setUp(self):`
		`super(TestUpload, self).setUp()`
		`@@ -1129,6 +1130,25 @@`
		`self.assertEqual(expected_sources_content,`
		`self.read_file(self.sources_file).strip().split('\n'))`

		`+ def test_upload_file_twice_but_checksum_is_different(self):`
		`+ self.write_file(os.path.join(self.cloned_repo_path, 'sources'),`
		`+ content='123456 README.rst')`
		`+`
		`+ readme_rst = os.path.join(self.cloned_repo_path, 'README.rst')`
		`+ self.write_file(readme_rst, content='Hello rpkg')`
		`+`
		`+ cli_cmd = [`
		`+ 'rpkg', '--path', self.cloned_repo_path, 'upload', readme_rst`
		`+ ]`
		`+ with patch('sys.argv', new=cli_cmd):`
		`+ cli = self.new_cli()`
		`+ with patch('pyrpkg.lookaside.CGILookasideCache.upload',`
onosek commented 5 years ago Quite unclear to me. Won't it work event without this second 'patch'? You have 2 files - sources and README.rst. I thought, purpose is to upload 2 different files with same name and catch exception.
cqi commented 5 years ago I thought, purpose is to upload 2 different files with same name and catch exception. Yes, you are right. :) The different files cause different hash calculated. So, for the purpose, this test first writes sources directly to simulate "README.rst was uploaded and the hash is 123456". Then, write arbitrary content to README.rst and run `upload` actually to catch the exception.
		`+ new=self.lookasidecache_upload):`
		`+ six.assertRaisesRegex(`
		`+ self, rpkgError,`
		`+ r'.+README.rst which has different checksum.+',`
		`+ cli.upload)`
		`+`

		`class TestSources(LookasideCacheMock, CliTestCase):`

cqi commented 5 years ago

The problem is, upload is able to upload a file, which is already in
sources with different checksum. This fix prevents from uploading that
file.

Fixes #204

Signed-off-by: Chenxiong Qi cqi@redhat.com

lsedlar commented on line 18 of pyrpkg/sources.py 5 years ago

Can you change the error to ... please use new-sources or remove the file from sources file manually.? If someone needs to have multiple sources, new-sources would remove all of them, which could be confusing.

lsedlar commented 5 years ago

Looks good to me.

rebased onto 90973446416e14163d4bda3535d100e5d4a70dee

5 years ago

rebased onto 51451ab0d989850ca43c5576fa75027219ad6e9a

5 years ago

onosek commented on line 19 of pyrpkg/sources.py 5 years ago

multiple - typo. 'are' is missing.

onosek commented on line 25 of tests/test_cli.py 5 years ago

Quite unclear to me. Won't it work event without this second 'patch'? You have 2 files - sources and README.rst. I thought, purpose is to upload 2 different files with same name and catch exception.

cqi commented on line 25 of tests/test_cli.py 5 years ago

I thought, purpose is to upload 2 different files with same name and catch exception.

Yes, you are right. :) The different files cause different hash calculated.

So, for the purpose, this test first writes sources directly to simulate "README.rst was uploaded and the hash is 123456". Then, write arbitrary content to README.rst and run upload actually to catch the exception.

rebased onto 4bd1e75

5 years ago

cqi commented 5 years ago