johannesboyne / gofakes3

A simple fake AWS S3 object storage (used for local test-runs against AWS S3 APIs)
MIT License
358 stars 81 forks source link

unmarshall to boltobject before listing #57

Closed ocakhasan closed 3 years ago

ocakhasan commented 3 years ago

Hi,

We are using gofakes3 testing for s5cmd so thank you for this amazing project. When we are testing, we noticed something probably a bug.

gofakes3 shows object sizes larger than it should be.

For example, let's say we have a file named file.txt

> echo "this is a file content" > file.txt

If we check the size it says, it is 23 bytes.

> ls -lah file.txt
-rw-r--r-- 1 hasan hasan 23 Aug 17 10:50 file.txt

But in s5cmd, if we put this object to a bucket, then get object with ListBucket method. It will give us size of 317 bytes. I tried to debug the issue and find out:

  1. when gofakes3 puts an object, first it converts whole object to an array using bson.Marshall method.
  2. Then it puts the whole converted array to object
  3. When listing objects it puts the size of whole array, therefore, gofakes3 shows file size larger than it should be. You can check it from here.

My suggestion is that we need to unmarshall the converted array, then use this unmarshalled object.

gofakes3 uses unmarshall just before GetObject function. And the results are correct in that case (file size is correct etc). You can check it from here. I think if we do the same process in ListBucket function, we will have no problem.

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

codecov-commenter commented 3 years ago

Codecov Report

Merging #57 (51aef96) into master (6a9f95c) will increase coverage by 1.98%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   65.90%   67.89%   +1.98%     
==========================================
  Files          28       28              
  Lines        2241     2492     +251     
==========================================
+ Hits         1477     1692     +215     
- Misses        530      564      +34     
- Partials      234      236       +2     
Impacted Files Coverage Δ
validation.go 85.71% <0.00%> (-5.20%) :arrow_down:
messages.go 70.75% <0.00%> (-1.16%) :arrow_down:
internal/goskipiter/iter.go 73.91% <0.00%> (-1.09%) :arrow_down:
time.go 86.66% <0.00%> (-0.84%) :arrow_down:
log.go 100.00% <0.00%> (ø)
chunk.go 100.00% <0.00%> (ø)
option.go 70.00% <0.00%> (ø)
backend.go 100.00% <0.00%> (ø)
backend/s3afero/hash.go 0.00% <0.00%> (ø)
backend/s3afero/option.go 0.00% <0.00%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6a9f95c...51aef96. Read the comment docs.

ocakhasan commented 3 years ago

hey @johannesboyne can you have a look please?

johannesboyne commented 3 years ago

Hi @ocakhasan thanks a lot for the feedback and kind words! Glad that the project helps ;-) And even more important, thanks a lot for the contribution it looks like a pretty good catch and something we should change!

johannesboyne commented 3 years ago

Already merged, but maybe one additional ask, as you've already spotted the bug - if you would be open to write a small test case for it, we can prevent regression of that topic.