DefaultContextResolver should respect the defaultRealm configuration

[eric-maynard]

Description

Previously DefaultContextResolver would always use default-realm as the default realm regardless of the configuration for defaultRealm. This PR fixes this behavior.

Fixes #281

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• Documentation update
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

Please delete options that are not relevant.

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings

[flyrain]

Thanks @eric-maynard for working on it! Left some comments. Let me know if they make sense. Also this changes the behavior a bit, I believe it's worthwhile to add a test.

[flyrain]

Can we combine these two fields(defaultRealm, realmContextResolver) to one? I don't see a reason to keep both of them. I'd prefer to keep the resolver, which may have different implementations.

What we can do is to mimic the rate limiter. The default realm name can be picked up by the implementation itself, instead of setting a string field then pass it to the resolver.

  @JsonProperty("rateLimiter")
  public RateLimiter getRateLimiter() {
    return rateLimiter;
  }

  @JsonProperty("rateLimiter")
  public void setRateLimiter(@Nullable RateLimiter rateLimiter) {
    this.rateLimiter = rateLimiter;
  }

[eric-maynard]

I'm confused by this suggestion; the linked issue is about users not being able to pass an explicit string default via the config. Are you suggesting that we remove the config?

[flyrain]

ah, I wasn't clear in the first place. I think we can combine these two configs to one.

realmContextResolver:
  type: default

defaultRealms:
  - default-realm

To

realmContextResolver:
  type: default
  defaultRealms: default-realm

With that, we don't have to keep two fields in the config class, and I think it is a nice way to manage the lifecycle of realmContextResolver. It could be immutable object without any setting methods.

[flyrain]

If we made the above change, I think we don't have to make the interface changes in RealmContextResolver. We just need to add a constructor in the class DefaultContextResolver like this

public DefaultContextResolver( @JsonProperty("defaultRealms") final String defaultRealm) {
    this.defaultRealm = defaultRealm;
}

Not setter and getter are needed in this way. WDYT?

[eric-maynard]

Removing the default constructor is still a breaking change, so is this materially better?

[eric-maynard]

Also, to be clear, we are not just changing how this works for DefaultContextResolver but for all RealmContextResolver implementations

[sfc-gh-ygu]

It is cleaner solution unless the config defaultRealm would be used by other places than RealmContextResolver. Do we?

[eric-maynard]

The defaultRealm is just the default realm used by RealmContextResolver when the realm cannot be parsed from the request

[flyrain]

Synced with @eric-maynard offline, we agreed on pushing this break change to another PR.

[flyrain]

Thanks @eric-maynard for the fix!