Extract rate limiter configuration properties into separate classes - #4242
Open
AryamannSingh7 wants to merge 1 commit into
Open
Conversation
RedisRateLimiter and RequestRateLimiterGatewayFilterFactory were themselves @ConfigurationProperties beans, but neither has a no-arg constructor because their collaborators are constructor injected. On a refresh, ConfigurationPropertiesRebinder cannot build a throwaway defaults instance for such a bean, so it skips resetting the properties to their class defaults before rebinding. A property removed from the environment therefore keeps its previously bound value rather than reverting. Move the bound state onto dedicated RedisRateLimiterProperties and RequestRateLimiterProperties classes, which have default constructors and so can be reset and rebound normally. The property prefixes are unchanged, so no configuration keys change for users, and the existing accessors are retained as deprecated delegates. RedisRateLimiter also inherits a bindable per-route config map from AbstractStatefulConfigurable, which was populated only because the rate limiter was itself the @ConfigurationProperties bean. That map now lives on RedisRateLimiterProperties and RedisRateLimiter#getConfig() reads through to it, so redis-rate-limiter.config.* binds exactly as before and is now rebound on refresh as well. Closes spring-cloudgh-4233 Signed-off-by: Aryamann Singh <107678802+AryamannSingh7@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes gh-4233.
Background
@spencergibb — following your direction in this comment ("I like number three, but also extract separate configuration properties classes").
The commons half is already done.
679cf202("Skip resetting beans to default values if there is no default constructor") added ahasDefaultConstructorguard toConfigurationPropertiesRebinder.resetBeanToDefaults, closing spring-cloud/spring-cloud-commons#1700. I checked it A/B: againstspring-cloud-context5.0.2 the WARN from the issue fires, against 5.0.3-SNAPSHOT it is silent, loggingNo default constructor for ...; skipping property reset before rebindingat debug instead. Since gateway builds against 5.0.3-SNAPSHOT, that warning should already be gone here.So this PR is only the extraction.
Why it still matters
The commons guard only changes the log level — the reset is skipped either way. So a property removed from the environment does not revert to its class default on refresh; it keeps its previously bound value.
RedisRateLimiterandRequestRateLimiterGatewayFilterFactoryare the only two@ConfigurationPropertiesbeans in the webflux server without a no-arg constructor (SetStatusGatewayFilterFactory,XForwardedHeadersFilterandRemoveHopByHopHeadersFilterall have one), so they are the only two affected.What changed
RedisRateLimiterPropertiesandRequestRateLimiterProperties, both with default constructors, holding the state that was previously bound onto the beans themselves.Preserving
redis-rate-limiter.config.*Worth calling out separately, since it is not obvious from the diff.
RedisRateLimiterinheritspublic Map<String, Config> getConfig()fromAbstractStatefulConfigurable. That map was bindable only because the rate limiter was itself the@ConfigurationPropertiesbean — so moving the header/flag fields out and dropping the annotation would have silently brokenspring.cloud.gateway.server.webflux.redis-rate-limiter.config.<routeId>.*.The map therefore now lives on
RedisRateLimiterProperties, andRedisRateLimiter#getConfig()reads through to it. Binding is unchanged from a user's point of view, and it is now rebound on refresh as well.RateLimiterPropertiesBindingTests#perRouteConfigIsStillBoundcovers this — it fails if the map is left on the rate limiter.Open questions
Both carried over from my earlier comment on the issue. Happy to change either — neither is baked in deeply:
setIncludeHeadersand friends,isDenyEmptyKey,isThrowOnLimit) are public API. I kept them as@Deprecated(since = "5.0.3")delegates. Would you rather they were removed outright in 5.x?@ConfigurationPropertiesbeans alone, since they have no-arg constructors and are not affected. Extend the same treatment to them for consistency, or leave them?Testing
Added
RateLimiterPropertiesBindingTests(binding, injection, and the per-route config case above) plus unit tests for each new properties class. ExistingRedisRateLimiterTests,RedisRateLimiterLuaScriptTests,RedisRateLimiterConfigTests,RequestRateLimiterGatewayFilterFactoryTestsandGatewayAutoConfigurationTestspass unchanged.One note:
docs/modules/ROOT/partials/_configprops.adocis generated, so I left it untouched. Regenerating it should drop therequest-rate-limiter.default-key-resolverand.default-rate-limiterentries, which came from getter-only metadata on the filter factory rather than from bindable properties.