Skip to content

Fix S3 folder creation for new AWS API#11726

Merged
MorrisJobke merged 1 commit intomasterfrom
s3-fixunittests
Oct 23, 2014
Merged

Fix S3 folder creation for new AWS API#11726
MorrisJobke merged 1 commit intomasterfrom
s3-fixunittests

Conversation

@PVince81
Copy link
Contributor

This also fixes the unit tests

Fixes #11725

S3 unit tests now all pass for me locally.

This also fixes the unit tests
@PVince81
Copy link
Contributor Author

Please review @LukasReschke @butonic @helmutschneider @icewind1991

@PVince81
Copy link
Contributor Author

Basically the new API requires all objects to have a body... in our case we abuse the object store by creating dummy entries for folders. There folders now need to have a (empty) body as well.

@PVince81
Copy link
Contributor Author

I tested creating a folder over the UI and it also worked fine.

@helmutschneider
Copy link
Contributor

Very strange. In my PR (#10680) to upgrade the sdk I fixed this. Seems it was failing later though: 0d0c9d0

@PVince81
Copy link
Contributor Author

Not sure, maybe some other change sneaked in and broke it somehow.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@PVince81
Copy link
Contributor Author

Just tried 993376f (before this PR) and it's not possible to create folders in the UI. There's an exception "You must specify a non-null value for the Body or SourceFile parameters."

@PVince81
Copy link
Contributor Author

@helmutschneider your code from d2979da seems correct. It seems your change possibly got lost in a merge conflict resolution 😕

@helmutschneider
Copy link
Contributor

@PVince81 No, that code is only relevant to the touch action (which I noticed was failing). My local tests did not fail on mkdir though which is really weird. Also unlikely that amazon changed their S3 api :/

@PVince81
Copy link
Contributor Author

@helmutschneider I found the reason: https://github.com/owncloud/core/pull/11375/files#r19245328

There was that other PR to stable7 to fix stuff. When I ported it to master I didn't know I needed to put the "Body" back.

@PVince81
Copy link
Contributor Author

@helmutschneider it seems the file already had the correct code when you made your change: https://github.com/helmutschneider/core/blob/d2979da864bd7b9759a0efb60e6eed62d79fe473/apps/files_external/lib/amazons3.php#L151
This explains why the unit tests passed back then.

The "Body" part likely got lost when I forward ported #11375

@PVince81
Copy link
Contributor Author

Anyway, if the change makes sense (and runs for you) I'd appreciate a thumbs up for the review 😄

@helmutschneider
Copy link
Contributor

@PVince81 I don't have time to test right now but I trust your judgement. Code makes sense too. Well done! 👍

@ghost
Copy link

ghost commented Oct 22, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/1001/
🚀 Test PASSed. 🚀

@PVince81
Copy link
Contributor Author

Thanks!

@PVince81 PVince81 added this to the 2014-sprint-06-current milestone Oct 22, 2014
@LukasReschke
Copy link
Member

👍

MorrisJobke added a commit that referenced this pull request Oct 23, 2014
Fix S3 folder creation for new AWS API
@MorrisJobke MorrisJobke merged commit ca01530 into master Oct 23, 2014
@MorrisJobke MorrisJobke deleted the s3-fixunittests branch October 23, 2014 10:35
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing S3 unit tests on master / cannot create folder on S3

5 participants