Support Git-style searchPaths with wildcards in AWS S3 buckets (#2812)#2958
Support Git-style searchPaths with wildcards in AWS S3 buckets (#2812)#2958tomy8964 wants to merge 10 commits into
Conversation
…g-cloud#2812) Fixes spring-cloud#2812 - ListObjectsV2 + AntPathMatcher based matching for *, **, ?, dot-wildcard - auto-ext lookup order (.properties → .json → .yml/.yaml) - directory scan, deduplication, prefix extraction - only active when searchPaths non-empty Signed-off-by: Geonwook Ham <tomy8964@naver.com> Signed-off-by: ham <tomy8964@naver.com>
Signed-off-by: Geonwook Ham <tomy8964@naver.com>
|
Hi @ryanjbaxter! |
There was a problem hiding this comment.
Pull request overview
This PR extends the AWS S3 environment repository to support Git-style searchPaths (placeholders + wildcard matching), enabling S3-backed config layouts to match existing Git-backed repository conventions.
Changes:
- Add
searchPathsconfiguration to the AWS S3 backend and wire it through the factory. - Implement placeholder expansion and wildcard-based S3 key discovery (including directory scans) with deduplication.
- Add an extensive JUnit 5 test suite covering placeholder/wildcard resolution scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepository.java |
Adds searchPaths support, wildcard matching via AntPathMatcher, S3 list/head probing, and key→property source wrapping. |
spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryFactory.java |
Passes searchPaths from properties into the repository constructor. |
spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentProperties.java |
Introduces searchPaths as a bindable configuration property. |
spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/AwsS3EnvironmentRepositoryTests.java |
Adds coverage for placeholder/wildcard behavior, ordering, and special cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes spring-cloud#2812 Rework AwsS3EnvironmentRepository path resolution and profile handling. Reverse the app list before checking/appending the default application and only append the default when it is missing. Simplify property-source iteration and invoke negated-profile property source logic only when searchPaths is empty to avoid duplicate sources. Normalize search path patterns by resolving null label/profile vars, collapsing duplicate slashes and trimming leading '/', and optimize extension probing so existing extensions aren't re-probed. Signed-off-by: Geonwook Ham <tomy8964@naver.com>
ad8fc5a to
daa088a
Compare
Fixes spring-cloud#2812 In AwsS3EnvironmentRepositoryTests, avoid mutating a shared 'server' instance by creating a new ConfigServerProperties (serverWithDefaultLabel) and setting its defaultLabel before constructing AwsS3EnvironmentRepository. This prevents side-effects on shared test state and ensures the repository gets the intended default label for the test. Signed-off-by: Geonwook Ham <tomy8964@naver.com>
|
Hi @ryanjbaxter! I have successfully addressed all the feedback from the Copilot review:
The checks are passing, and it's ready for your review. Could you please take another look when you have some time? Thank you! |
The for loop header and the first statement are on the same line (for (...) {String resolvedLabel = ...), which is likely to fail formatting checks (and is hard to read). Split the loop body onto its own lines.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Geonwook Ham <tomy8964@naver.com>
Fixes spring-cloud#2812 Generalize getS3ConfigFileWithSearchPaths by adding a Function keyWrapper to handle different key-wrapping strategies (profile-specific and negated-profile cases). Implement wrapKeyWithNegatedConfigFiles to support YAML documents with negated spring.config.activate.on-profile expressions and wire it into property source discovery. Clean up formatting, streamline S3 client calls, and simplify create/wrapper methods. Also remove explicit setOrder in AwsS3EnvironmentRepositoryFactory (return repository directly) and update tests for minor formatting/usage changes. Signed-off-by: Geonwook Ham <tomy8964@naver.com>
|
Hi @ryanjbaxter! I have successfully addressed the second round of feedback from the Copilot review:
The checks are passing again, and the codebase is now much cleaner and more reusable. Could you please take another look when you have some time? Thank you! |
|
|
||
| public AwsS3EnvironmentRepository(S3Client s3Client, String bucketName, boolean useApplicationAsDirectory, | ||
| ConfigServerProperties server) { | ||
| this(s3Client, bucketName, useApplicationAsDirectory, server, null); |
There was a problem hiding this comment.
Could we pass an empty list here?
There was a problem hiding this comment.
Yes, we updated this constructor to pass Collections.emptyList() instead of null.
|
|
||
| private int order = DEFAULT_ORDER; | ||
|
|
||
| private List<String> searchPaths = new ArrayList<>(); |
There was a problem hiding this comment.
This could be Collections.emptyList()
There was a problem hiding this comment.
Yes, we initialized searchPaths using Collections.emptyList() to optimize memory usage and guarantee immutability by default.
| boolean fileFound = false; | ||
| List<String> extensionsToProbe = (pattern.endsWith(".properties") || pattern.endsWith(".json") | ||
| || pattern.endsWith(".yml") || pattern.endsWith(".yaml")) ? List.of("") | ||
| : List.of(".properties", ".json", ".yml", ".yaml"); |
There was a problem hiding this comment.
This could be private static final
There was a problem hiding this comment.
Yes, we extracted these extension lists into private static final constants (SUPPORTED_EXTENSIONS and EMPTY_EXTENSION) at the class level.
|
|
||
| if (pattern.endsWith(".*")) { | ||
| String base = pattern.substring(0, pattern.length() - 2); | ||
| for (String ext : List.of(".properties", ".json", ".yml", ".yaml")) { |
There was a problem hiding this comment.
extentions could be reused here
There was a problem hiding this comment.
Yes, we replaced it to reuse the static SUPPORTED_EXTENSIONS constant.
| } | ||
|
|
||
| String expression = onProfileValue.toString().trim(); | ||
| if (!expression.contains("!") && !expression.contains("&") && !expression.contains("|") |
There was a problem hiding this comment.
Couldnt you use isSimpleProfileName?
There was a problem hiding this comment.
Yes, we refactored isSimpleProfileName to be a package-private static helper method and reused it here to check for simple profile names.
| boolean useApplicationAsDirectory, S3Client s3Client, boolean callReadImmediately) { | ||
| super(application, profile, label, bucketName, useApplicationAsDirectory, s3Client); | ||
| this.properties = read(); | ||
| if (callReadImmediately) { |
There was a problem hiding this comment.
Is this necessary? There are subclasses that now call read immediately, so could this implementation just not call read in the constructor? And if so I would prefer to introduce a new class which has the new functionality (not call read immediately) and then leave these as is.
There was a problem hiding this comment.
We refactored this entire part. Instead of adding a callReadImmediately flag to the base classes, we consolidated all three key-based config file classes (PropertyConfigFileFromKey, YamlConfigFileFromKey, JsonConfigFileFromKey) into a single S3ConfigFileFromKey class. The base classes remain unchanged (calling read() in the constructor as they originally did), and S3ConfigFileFromKey handles reading appropriately, allowing us to remove the callReadImmediately flag entirely.
ryanjbaxter
left a comment
There was a problem hiding this comment.
Could you also add documentation?
|
|
||
| AwsS3EnvironmentRepository repository = new AwsS3EnvironmentRepository(client, | ||
| environmentProperties.getBucket(), environmentProperties.isUseDirectoryLayout(), server); | ||
| repository.setOrder(environmentProperties.getOrder()); |
There was a problem hiding this comment.
Why is setOrder no longer called?
There was a problem hiding this comment.
We introduced a new constructor in AwsS3EnvironmentRepository that accepts AwsS3EnvironmentProperties directly. Inside this constructor, the order is mapped and set directly from the properties (this.order = properties.getOrder()), making the repository initialization cleaner in the factory.
| String label) { | ||
| List<S3ConfigFile> s3ConfigFiles = getS3ConfigFile(app, profile, label, | ||
| this::getNonProfileSpecificPropertiesOrJsonConfigFile, this::getNonProfileSpecificS3ConfigFileYaml); | ||
| List<S3ConfigFile> s3ConfigFiles = searchPaths.isEmpty() ? getS3ConfigFile(app, profile, label, |
There was a problem hiding this comment.
When searchPaths is set why are nonProfileSpecificPropertiesOrJsonConfigFile not called? Wouldn't this result in application.* files not being included?
There was a problem hiding this comment.
When searchPaths is active, the wildcard directory scanning in addProfileSpecificPropertySource automatically discovers both profile-specific and non-profile-specific files (e.g. application.yml and application-profile.yml) matching the patterns. For each YAML file found, it loads both profile-specific and non-profile-specific documents. Skipping it here prevents duplicate S3 API calls and duplicate property sources in the environment.
| try { | ||
| s3Client.headObject(HeadObjectRequest.builder().bucket(bucketName).key(key).build()); | ||
| result.addAll(keyWrapper.apply(key)); | ||
| break; |
There was a problem hiding this comment.
Wouldn't this stop the for loop after the first file is found and we wouldn't check the rest of the extensions?
There was a problem hiding this comment.
Yes, that was a mistake in the previous version. We removed the break statement so that all matching files with different extensions (e.g., both .properties and .yml at the same path) are scanned and loaded, aligning with Git/Native backend behavior.
| catch (S3Exception e) { | ||
| int status = e.statusCode(); | ||
| if (status != 404 && status != 403) { | ||
| throw e; |
There was a problem hiding this comment.
Might be worth logging this as INFO in case there is a legitimate configuration error occurring and not just that the file does not exist
There was a problem hiding this comment.
Yes, we added an INFO level log statement (LOG.info("Error checking S3 object key: " + key, e);) before rethrowing the exception to make troubleshooting configuration errors easier.
|
|
||
| } | ||
|
|
||
| private List<S3ConfigFile> getS3ConfigFileWithSearchPaths(String application, String profile, String label, |
There was a problem hiding this comment.
I would prefer this method be broken up into small more digestible chunks to make it easier to reason about and maintain/test
There was a problem hiding this comment.
Yes, we broke down getS3ConfigFileWithSearchPaths into smaller helper methods (probeLiteralPattern, scanDirectoryPattern, probeDotWildcardPattern, scanWildcardPattern) to improve readability and testability.
|
|
||
| } | ||
|
|
||
| class PropertyConfigFileFromKey extends PropertyS3ConfigFile { |
There was a problem hiding this comment.
These three classes are nearly identical. That combined with the callReadImmediately comment I had above makes it feel like these classes could be refactored
There was a problem hiding this comment.
Yes, we completely refactored this by consolidating PropertyConfigFileFromKey, YamlConfigFileFromKey, and JsonConfigFileFromKey into a single S3ConfigFileFromKey class.
| this(s3Client, bucketName, useApplicationAsDirectory, server, null); | ||
| } | ||
|
|
||
| public AwsS3EnvironmentRepository(S3Client s3Client, String bucketName, boolean useApplicationAsDirectory, |
There was a problem hiding this comment.
It seems better at this point to just have a constructor that takes in AwsEnvironmentProperties
There was a problem hiding this comment.
Yes, we added a new constructor AwsS3EnvironmentRepository(S3Client, AwsS3EnvironmentProperties, ConfigServerProperties) that delegates to the canonical constructor, simplifying the factory builder.
| "default", "defaultlabel", null, new String[] { "s3://test/defaultlabel" })); | ||
| } | ||
|
|
||
| // 1) Placeholder only |
There was a problem hiding this comment.
We dont need the number comments on the tests
There was a problem hiding this comment.
Yes, we removed the numbered comments from the test cases.
…dant callReadImmediately flags Signed-off-by: Geonwook Ham <tomy8964@naver.com>
Signed-off-by: Geonwook Ham <tomy8964@naver.com>
|
Hi @ryanjbaxter! Summary of Changes
|
Summary
Add full Git-style
searchPathssupport (placeholders + wildcards) to the AWS S3 backend so that users can migrate existing Git-based configurations without renaming toapplication.*..*), single (?) and double (**) wildcard matching.properties → .json → .yml → .yamlwhen expanding literals or.*patternsThis issue (#2812)
resolves #2812
Changes
Core Logic
Extended
getS3ConfigFileWithSearchPaths(...)to treat{application}and{profile}placeholders identically to the Git backendAdded support for:
.*): expands against supported extensions in priority order?)**)/Ensured seenKeys set to avoid duplicate property sources
Tests
Added new JUnit 5 tests in
AwsS3EnvironmentRepositoryTeststo cover all scenarios:searchPaths_placeholderOnly_shouldResolveExactFilesearchPaths_wildcardOnly_shouldResolveAllPropertiessearchPaths_placeholderAndWildcard_shouldResolveMatchingKeyssearchPaths_orderMatters_forPropertySourceOrdersearchPath_extensionPreserved(dynamic tests for each extension)searchPaths_applicationAsDirectory_shouldStillHonorSearchPathsmultiDocumentYaml_withSearchPaths_shouldNotSplitDocumentsgetLocations_returnsCorrectsearchPaths_deduplication_shouldOnlyAddOncesearchPaths_singleCharacterWildcard_shouldMatchExactlyOneCharsearchPaths_withEmptyLabel_shouldUseDefaultLabelsearchPaths_multipleLabels_shouldApplyForEachLabelInReverseOrdersearchPaths_literalNotFound_shouldReturnEmpty…plus the additional edge cases for literal-stop, dot-wildcard priority, and nested directory patterns.
Example Usage
Additional Notes
Backward Compatibility
This update does not break any existing AWS S3 config setups. The default lookup behavior is unchanged if no placeholders or wildcards are used in
searchPaths.Migration
Existing users migrating from Git-backed config servers can now use their current
searchPaths(including wildcards and placeholders) on S3 with no renaming or convention change required.Documentation
Documentation and usage examples for the enhanced
searchPathswill be updated in the relevant documentation files after the merge.If there are specific locations that require documentation updates, please let me know—I will be happy to update them.
Performance and Cost
Using wildcards (such as
*or**) in searchPaths may result in additional AWS S3 API calls (e.g., ListObjects), especially for large buckets or deeply nested directory patterns.This could lead to increased latency and higher AWS costs.
Users should consider the structure and size of their buckets when designing searchPaths and monitor AWS S3 usage accordingly.