#343 Avoid to upload a file with different checksum
Merged 3 years ago by cqi. Opened 3 years ago by cqi.

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):

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',

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 3 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):

  

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

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.

rebased onto 90973446416e14163d4bda3535d100e5d4a70dee

3 years ago

rebased onto 51451ab0d989850ca43c5576fa75027219ad6e9a

3 years ago

multiple - typo. 'are' is missing.

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.

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

3 years ago

multiple - typo. 'are' is missing.

@onosek Done.

Thanks, let's merge it when ready.

Commit 17c5373 fixes this pull-request

Pull-Request has been merged by cqi

3 years ago

Pull-Request has been merged by cqi

3 years ago