Allow in-memory client registration repos to be constructed with a map

[vpavic]

This would allow to back InMemoryClientRegistrationRepository with a distributed Map, such as Hazelcast's IMap for instance.

Ideally, the implementations should be renamed as well in order to reflect their nature (e.g. to MapClientRegistrationRepository). In that case, to ensure backwards compatibility the InMemoryClientRegistrationRepository that would be provided as an implementation that simply delegates to MapClientRegistrationRepository and is marked as deprecated. Let me know what's your opinion on this, and if accepted should I do it via this PR or another one later on.

[jgrandja]

@vpavic This makes sense to allow InMemoryClientRegistrationRepository to be directly supplied with a Map - that may be a distributed one.

However, I'm not keen on deprecating InMemoryClientRegistrationRepository. Is there a specific reason you are wanting it deprecated? I do agree that InMemoryClientRegistrationRepository should delegate to the proposed new MapClientRegistrationRepository.

Please go ahead and add MapClientRegistrationRepository to this PR.

[jgrandja]

Also, what are your thoughts on just adding MapClientRegistrationRepository and leaving InMemoryClientRegistrationRepository as-is?

[vpavic]

Is there a specific reason you are wanting it deprecated?

It is a naming concern. If something can be backed by a distributed map like Hazelcast's IMap, and having in mind that Hazelcast can operate in client-server topology, then it isn't an in-memory implementation anymore. Besides, the term in-memory is vague, as multiple data structures can theoretically be used to fit that description. OTOH MapClientRegistrationRepository is very clear.

Also, what are your thoughts on just adding MapClientRegistrationRepository and leaving InMemoryClientRegistrationRepository as-is?

It sounds like an unnecessary duplication to me. With MapClientRegistrationRepository in, the InMemoryClientRegistrationRepository isn't needed anymore and I'd have it simply delegate to the new MapClientRegistrationRepository.

[jgrandja]

Besides, the term in-memory is vague, as multiple data structures can theoretically be used to fit that description.

The naming convention InMemory is pretty clear. The associated in-memory implementation is designed to be non-persistent and backed by an in-memory construct. These implementations are meant to be simple and usually considered default implementations. See also InMemoryOAuth2AuthorizedClientService and InMemoryUserDetailsManager.

It is a naming concern. If something can be backed by a distributed map like Hazelcast's IMap, and having in mind that Hazelcast can operate in client-server topology, then it isn't an in-memory implementation anymore

Agreed that this can be confusing if the user were to implement this way. An alternative implementation the user can do is:

@Bean
public ClientRegistrationRepository clientRegistrationRepository() {
	// Obtain registrations from Hazelcast's `IMap`
	final Map<String, ClientRegistration> registrations = getRegistrations();
	return registrationId -> registrations.get(registrationId);
}

OR

@Bean
public ClientRegistrationRepository clientRegistrationRepository() {
	// Obtain registrations from Hazelcast's `IMap`
	final Map<String, ClientRegistration> registrations = getRegistrations();
	return new MapClientRegistrationRepository(getRegistrations());
}

private class MapClientRegistrationRepository implements ClientRegistrationRepository, Iterable<ClientRegistration> {
	private Map<String, ClientRegistration> registrations;

	private MapClientRegistrationRepository(Map<String, ClientRegistration> registrations) {
		this.registrations = registrations;
	}

	@Override
	public ClientRegistration findByRegistrationId(String registrationId) {
		return this.registrations.get(registrationId);
	}

	@Override
	public Iterator<ClientRegistration> iterator() {
		return this.registrations.values().iterator();
	}
}

Do you think both of these alternative implementations are straight forward?

[rwinch]

Agreed that this can be confusing if the user were to implement this way. An alternative implementation the user can do is:

The issue is that this is not dynamic. The values in Hazelcast may be updated after this initialization is done. Also it may be too large to fit in memory.

I think it makes sense for the deprecation as migration is straight forward.

[jgrandja]

The issue is that this is not dynamic. The values in Hazelcast may be updated after this initialization is done. Also it may be too large to fit in memory.

The sample provided was only to demonstrate how easy it is to create a ClientRegistrationRepository backed by a Map (local or distributed). I understand your points, but the code sample is not making the assumption that the values would be initialized from the remote store and into local storage. It may have been interpreted that way but that was not my intention. You can assume that it's a reference to the external Map.

Either way, I still feel deprecating InMemoryClientRegistrationRepository is not really necessary given that a Map implementation can easily be created by the user with minimal code.

If we were to deprecate, than what do we do with InMemoryOAuth2AuthorizedClientService and InMemoryUserDetailsManager? Wouldn't the same rules apply?

[rwinch]

I think we would deprecate on an as needed (requested) basis from users.

[jgrandja]

@vpavic I spoke to @rwinch and we decided to not deprecate at this time. If we were to go that route than we would need to deprecate InMemoryClientRegistrationRepository and InMemoryReactiveClientRegistrationRepository and that would also trigger changes in Boot auto-configuration at a later point in time. It may not be an ideal name when a distributed Map is backed but at the same time it may be a locally backed Map which would be fine.

We did agree that InMemory* naming convention will not be used on a go forward basis.

The PR looks good. I'll get this merged shortly.

[vpavic]

Thanks for the follow up and sorry for my lack activity on this over the past week or so - I was just about to follow up myself.

We did agree that InMemory* naming convention will not be used on a go forward basis.

That's good to hear - hopefully at some later point in the 5.x lifecycle you would consider deprecation in favor of Map*.

[jgrandja]

Thanks for the PR @vpavic. This is now in master.