-
Notifications
You must be signed in to change notification settings - Fork 7
Enable conformance testing for GCP blob store #79
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
base: main
Are you sure you want to change the base?
Enable conformance testing for GCP blob store #79
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
============================================
+ Coverage 78.10% 78.21% +0.11%
- Complexity 1690 1701 +11
============================================
Files 136 136
Lines 6864 6890 +26
Branches 779 783 +4
============================================
+ Hits 5361 5389 +28
+ Misses 1079 1078 -1
+ Partials 424 423 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aa5a044
to
d051128
Compare
public void initializeWireMockServer() { | ||
harness = createHarness(); | ||
TestsUtil.startWireMockServer("src/test/resources", harness.getPort()); | ||
String path = String.format("src/test/resources", harness.getProviderId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't break the pattern, you can revert this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will revert this
|
||
// And run the tests given the invalid credentialsOverrider | ||
runOperationsThatShouldFail("testInvalidCredentials", bucketClient); | ||
if (harness.getProviderId() != GcpConstants.PROVIDER_ID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was created at the time when presigned URL generation was failing for GCP blob store. I will enable this and other presigned URL tests in a separate PR.
{ | ||
"id" : "f6149961-1023-4bb1-902a-c3f650a5c5a2", | ||
"name" : "storage_v1_b_substrate-sdk-gcp-poc1-test-bucket-versioned_o_conformance-tests_upload_happypath_versioned_path", | ||
"request" : { | ||
"url" : "/storage/v1/b/substrate-sdk-gcp-poc1-test-bucket-versioned/o/conformance-tests%2Fupload%2FhappyPath_versioned_Path", | ||
"method" : "DELETE" | ||
}, | ||
"response" : { | ||
"status" : 204, | ||
"headers" : { | ||
"Alt-Svc" : "h3=\":443\"; ma=2592000,h3-29=\":443\"; ma=2592000", | ||
"Server" : "UploadServer", | ||
"Cache-Control" : "no-cache, no-store, max-age=0, must-revalidate", | ||
"X-GUploader-UploadID" : "AAwnv3J6BCE-ay8JyAKYXE7-xn8MKpGbTAIqiP3VEdYILA6EF9OXXg8ckmNEBe-tDN_lnsLJ", | ||
"Vary" : [ "Origin", "X-Origin" ], | ||
"Pragma" : "no-cache", | ||
"Expires" : "Mon, 01 Jan 1990 00:00:00 GMT", | ||
"Date" : "Fri, 03 Oct 2025 00:48:00 GMT", | ||
"Content-Type" : "application/json" | ||
} | ||
}, | ||
"uuid" : "f6149961-1023-4bb1-902a-c3f650a5c5a2", | ||
"persistent" : true, | ||
"insertionIndex" : 1 | ||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be in mappings, something is not right with the local configs while recording
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are some extra junk files, removed them. Please check mappings folder now.
|
||
@Override | ||
protected void doDelete(String key, String versionId) { | ||
validateBucketExists(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this validation separately? it's another call for any given operation. Isn't gcp throw resource not found otherwise ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, GCP does not throw an exception if bucket does not exist, rather the delete API just passes with a False return value. The integration test expects explicit exception to be thrown.
|
||
return new Iterator<>() { | ||
private final Iterator<Blob> it = blobs.iterator(); | ||
Iterator<Blob> filteredIterator = applyDelimiterFilter(blobs, request.getPrefix(), request.getDelimiter()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why aren't we leveraging delimeter from list call ?
https://cloud.google.com/storage/docs/json_api/v1/objects/list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look at this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of GCS APIs with delimiter for filtering is not consistent. We are already making use of the delimiter and sending it as an API parameter. However, the expected filtering results are achieved only when the delimiter is slash(/), non-slash delimiters are not handled the same way as slash delimiter.
One example is: When the query is with delimiter="-"
and prefix="conformance-tests/blob-for-list/prefix"
, list of keys of the available items is ["conformance-tests/blob-for-list", "conformance-tests/blob-for-list/prefix-1", "conformance-tests/blob-for-list/prefix-2", "conformance-tests/blob-for-list/prefix_3"]
, expected result is ["conformance-tests/blob-for-list/prefix_3"]
, but the GCS API call returns ["conformance-tests/blob-for-list/prefix-" "conformance-tests/blob-for-list/prefix_3"]
.
Another way to handle this is to relax/remove the integration test for this particular case.
d051128
to
dac269c
Compare
blob/blob-client/pom.xml
Outdated
<dependency> | ||
<groupId>com.salesforce.multicloudj</groupId> | ||
<artifactId>multicloudj-common-gcp</artifactId> | ||
<scope>test</scope> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not right dependency, client cannot depend on substrate specific dependencies
import com.salesforce.multicloudj.blob.driver.UploadResponse; | ||
import com.salesforce.multicloudj.common.exceptions.InvalidArgumentException; | ||
import com.salesforce.multicloudj.common.exceptions.SubstrateSdkException; | ||
import com.salesforce.multicloudj.common.gcp.GcpConstants; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also not a right dependency, since this a temp workaround, just have a constant local and let's not introduce anti-pattern dependencies
No description provided.