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

Object metadata should be optional in resumable uploads #332 #699

Merged
merged 5 commits into from
Mar 8, 2022

Conversation

martinzink
Copy link
Contributor

We are working on google-cloud integration for our project Apache Nifi Minifi CPP and fake-gcs-server seems to be an awesome way to create integration tests.
However I encountered some issues, which made impossible to integrate fake-gcs-server with a C++ app

I already fixed the issues from google-cloud-cpp's side
googleapis/google-cloud-cpp#8446
googleapis/google-cloud-cpp#8477

The last issue that blocks using fake-gcs-server with a cpp application seems to be #332
As described in the issue fake-gcs-server is stricter than google cloud storage when it comes to resumable upload requests.
With this PR the object metadata is optional when creating a resumable upload request.

I also added cpp examples so it can be verified that the resumable uploads are working now with fake-gcs-server.

fakestorage/upload.go Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
#include "google/cloud/storage/client.h"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this to CI?

For that we need:

  1. A script in ./ci
  2. A docker image where that script can run (probably something with cmake preinstalled?)
  3. Then add it to the matrix here:
    matrix:
    include:
    - lang: python
    docker-image: python:latest
    entrypoint: /bin/bash
    - lang: node
    docker-image: node:14-alpine
    entrypoint: /bin/sh
    - lang: go
    docker-image: golang:alpine
    entrypoint: /bin/sh
    - lang: dotnet
    docker-image: mcr.microsoft.com/dotnet/sdk:5.0-alpine
    entrypoint: /bin/sh
    - lang: scala
    docker-image: mozilla/sbt:latest
    entrypoint: /bin/sh
    - lang: java
    docker-image: openjdk:11
    entrypoint: /bin/sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize that the examples are run by the CI, but that's awesome reworked the example so it is now a test.
c5f55f7
Unfortunately I didn't find any docker image that has the google-cloud-cpp already built (that could change as the fixes are now included the latest v1.37.0 release), so I had to build it from source that makes the cpp test a bit longer than the other languages.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh 3 minutes isn't too bad given that CI runs things in parallel and running the tests on Windows takes 6 minutes anyway. We could definitely optimize it later though (would probably require maintaining a custom image, or leverage some caching mechanism in GitHub Actions).

@@ -327,10 +327,7 @@ func (s *Server) multipartUpload(bucketName string, r *http.Request) jsonRespons
func (s *Server) resumableUpload(bucketName string, r *http.Request) jsonResponse {
predefinedACL := r.URL.Query().Get("predefinedAcl")
contentEncoding := r.URL.Query().Get("contentEncoding")
metadata, err := loadMetadata(r.Body)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that demonstrates this behavior?

Also is it possible to retain this check if we narrow it to ignore specific kinds of errors, e.g., missing metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. I checked the problem only occurs when there is no body in the initial resumable upload request, so I've changed that we only load the metadata if there is a body, and we still throw if the load fails.

I also refactored the TestServerUndocumentedResumableUploadAPI so the test is performed with and without the body of the initial request.
8fe5502

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing and sorry for the delayed review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants