Skip to content
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

Introduce IndexResolver #557

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@vpavic
Copy link
Member

commented Jun 20, 2016

This PR introduces IndexResolver as a strategy interface for resolving index values in FindByIndexNameSessionRepository implementations.

This PR assumes IndexResolver implementations are single purpose, meaning implementations support single index name. Each FindByIndexNameSessionRepository implementation then in turn defines its set of supported IndexResolvers. For this purpose, a convenience CompositeIndexResolver is provided.

Out of the box the PrincipalNameIndexResolver is provided as the driver for this improvement, however each `FindByIndexNameSessionRepository`` implementation can easily provide own resolvers, depending on capabilities of backing technology.

This resolves #376.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Jun 20, 2016

@vpavic Thank you for signing the Contributor License Agreement!

* Indicate supported index name.
* @return the index name
*/
protected abstract String supportedIndexName();

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 21, 2016

Member

What do we do if it supports multiple indexes?

This comment has been minimized.

Copy link
@vpavic

vpavic Jun 21, 2016

Author Member

Well, this is basically just an abstract base class that simplifies implementing single-purpose IndexResolver. Nothing in the IndexResolver contract itself prevents the implementations from supporting multiple indexes, as demonstrated by CompositeIndexResolver.

* @param indexName the index name to resolve
* @return the resolved index value, {@code null} if none found
*/
String resolve(S session, String indexName);

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 21, 2016

Member

Perhaps the input should just be the session and the result that has a mapping of index name to index value. Is there any advantage in only supporting a single session attribute?

This comment has been minimized.

Copy link
@vpavic

vpavic Jun 21, 2016

Author Member

IMO that wouldn't be optimal solution since it implies execution of logic for all supported indexes. This way you extract only what you need in a given situation.

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 21, 2016

Member

How would implementations that support multiple indexes (i.e. Gemfire) use this API?

The SessionRepository could keep all the index names that need indexing. However, that means the SessionRepository and the IndexResolver configuration need to be kept in sync which is not ideal.

This comment has been minimized.

Copy link
@vpavic

vpavic Jun 21, 2016

Author Member

Each FindByIndexNameSessionRepository implementation would internally define its supported IndexResolvers, either using a CompositeIndexResolver or some other way. These would then get consumed according to repository needs, i.e. the same way duplicated principal name resolving logic is consumed now.

This approach allows having some resolvers that are reused across all repository implementations, such as PrincipalNameIndexResolver, as well as each implementation having its specific resolvers. If you would go the input should just be the session and the result that has a mapping of index name to index value route then you wouldn't be able to reuse this logic.

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 22, 2016

Member

either using a CompositeIndexResolver or some other way.

This requires another API because the current interface must have knowledge of the index name.

If you would go the input should just be the session and the result that has a mapping of index name to index value route then you wouldn't be able to reuse this logic.

Why not? Seems you could do this:

public interface IndexResolver<S extends Session> {
    Map<String,String> resolveIndexes(S session);
}

public class PrincipalNameIndexResolver<S extends Session> implements IndexResolver<S> {
    private SpelExpressionParser parser = new SpelExpressionParser();
    private String indexName = FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME;

    public String resolvePrincipal(Session session) {
        String principalName = session.getAttribute(FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME);
        if(principalName != null) {
            return principalName;
        }
        Object authentication = session.getAttribute("SPRING_SECURITY_CONTEXT");
        if(authentication != null) {
            Expression expression = parser.parseExpression("authentication?.name");
            return expression.getValue(authentication, String.class);
        }
        return null;
    }

    @Override
    public Map<String, String> resolveIndexes(S session) {
        String index = resolvePrincipal(session);
        if(index == null) {
            return Collections.emptyMap();
        }
        return Collections.singletonMap(indexName, index);
    }
}

public class GemfireIndexResolver<S extends Session> implements IndexResolver<S> {
    private PrincipalNameIndexResolver<S> principalResolver = new PrincipalNameIndexResolver<>();

    @Override
    public Map<String, String> resolveIndexes(S session) {
        Map<String, String> gemfireIndexes = resolveThem(session);
        gemfireIndexes.putAll(principalResolver.resolveIndexes(session));
        return gemfireIndexes;
    }

    private Map<String, String> resolveThem(S session) {
        // TODO
        return null;
    }
}

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 22, 2016

Member

Each FindByIndexNameSessionRepository implementation holds the knowledge of its supported indexes anyway, doesn't it?

It does. Introducing this new API we have yet another API that needs to be aware of the index names and be kept in sync. The proposed change allows all the index logic to be moved into the new resolver api.

Your GemfireIndexResolver will get messy once other IndexResolver resolvers get in there too since you're relying on the concrete IndexResolver implementation.

How would this be cleaner with the original design? Either way you would need to have that specific functionality.

you'd really like to run the resolving for all the indexes and then extract the single value you're interested in instead of current

When are we interested in only a single index?

This comment has been minimized.

Copy link
@vpavic

vpavic Jun 22, 2016

Author Member

I've updated the PR with changes to FindByIndexNameSessionRepository implementations to better demonstrate the intended usage of IndexResolvers

When are we interested in only a single index?

Well, at any given time you're attempting to resolve an index you're interested in a single value, right? Unless I'm missing something obvious.

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 22, 2016

Member

Most the time I want to write all the index's (that have changed) to the data store. This could easily be done with the suggested changes.

If I'm looking up an index, then the user provides the index name and the index value so I do not need to resolve anything.

This comment has been minimized.

Copy link
@vpavic

vpavic Jun 23, 2016

Author Member

But this is dependent on the technology used to implement the repository, isn't it? For example, consider the PrincipalNameExtractor from #544. It's basically a wrapper around an IndexResolver.

OK, it certainly makes sense to provide API to communicate all the supported indexes. What are you thoughts on making it a part of FindByIndexNameSessionRepository?

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 23, 2016

Member

It's basically a wrapper around an IndexResolver

Right, but the logic to resolve the index would be able to be reused. This is the goal. If we need adapters to translate the results of the IndexResolver into implementation specific code this is much easier than rewriting the code for resolving the indexes.

What are you thoughts on making it a part of FindByIndexNameSessionRepository?

  • This interface is already GA, so we cannot add a new method on it
  • I don't think this is the right place for it. User's do not care what the indexes are. It is the internal implementation that cares about the indexes. So the FindByIndexNameSessionRepository implementation would have an IndexResolver injected into it.

@vpavic vpavic force-pushed the vpavic:gh-376 branch from 7875cc1 to 255186d Jun 22, 2016

@vpavic vpavic referenced this pull request Jun 29, 2016

Closed

Add first-class Hazelcast support #544

4 of 4 tasks complete

@vpavic vpavic force-pushed the vpavic:gh-376 branch from 255186d to d1da4dd Jun 29, 2016

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2016

@rwinch I've updated the PR to address the concerns from our previous discussion.

IndexResolver now provides operations to indicate the supported indexes, and resolve all indexes. It also includes Delegate contract for delegates such as PrincipalNameIndexResolver.

FindByIndexNameSessionRepository implementations currently do not adopt these changes. I'll do that part after your review.

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 29, 2016

@vpavic Thanks for the update. This doesn't appear to be what I had in mind. Is there a reason we need anything other than:

public interface IndexResolver<S extends Session> {
    Map<String,String> resolveIndexes(S session);
}

as outlined in my comments?

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2016

Having the possibility to check whether given index name is supported by the resolver vastly simplifies the checks found in FindByIndexNameSessionRepository#findByIndexNameAndIndexValue implementations. This can now be handled via single call of IndexResolver#supportsIndex. I thought you referred to that by Introducing this new API we have yet another API that needs to be aware of the index names and be kept in sync.

Delegate spec simplifies the IndexResolver implementation, with the provided DefaultIndexResolver IMO covers all the use cases and avoids having ugly if-then-else logic in IndexResolver implementations.

Resolving a single index is also pretty common case IMO, there are two examples in RedisOperationsSessionRepository alone:

Also with this design we could easily support users adding their own indexes with some FindByIndexNameSessionRepository implementations - they would simply implement Delegate contract and register beans which we would then get automatically register with IndexResolver.

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@vpavic Thanks for your response.

Having the possibility to check whether given index name is supported by the resolver vastly simplifies the checks found in FindByIndexNameSessionRepository#findByIndexNameAndIndexValue implementations.

Ok. I see your point. Sorry for the confusion. It might be good to continue doing this sort of optimization.

I was planning on having the implementation just attempt to perform the lookup regardless of if the index was supported. If the value wasn't indexed, it would be a null session.

Do you see reason for having both supportedIndexes and supportsIndex method?

I'd really like to keep this API as simple as possible. I think it may be valuable to remove both the supportedIndexes and supportsIndex methods. It seems that we will have all the information we need with a single method.

Delegate spec simplifies the IndexResolver implementation, with the provided DefaultIndexResolver IMO covers all the use cases and avoids having ugly if-then-else logic in IndexResolver implementations.
...
Resolving a single index is also pretty common case IMO

Agreed. This is a common use case.

Could this be an abstract class rather than an interface?

Also with this design we could easily support users adding their own indexes with some FindByIndexNameSessionRepository implementations - they would simply implement Delegate contract and register beans which we would then get automatically register with IndexResolver.

We can do the same by having them provide multiple FindByIndexNameSessionRepository implementations. Right?

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2016

I was planning on having the implementation just attempt to perform the lookup regardless of if the index was supported. If the value wasn't indexed, it would be a null session.

I see, however I was driven by optimization considerations from the start - why run the resolver logic if we have enough knowledge to know it won't yield any result?

Do you see reason for having both supportedIndexes and supportsIndex method?

I agree we probably don't need both, supportsIndex is by far more relevant for a typical usage of IndexResolver.

I'd really like to keep this API as simple as possible. I think it may be valuable to remove both the supportedIndexes and supportsIndex methods. It seems that we will have all the information we need with a single method.

Could you please clarify this a bit more? What single method do you refer to if we remove the both support-methods? Or did you mean remove one of the methods rather than remove both?

Could this be an abstract class rather than an interface?

Yes, I've also considered such approach.

We can do the same by having them provide multiple FindByIndexNameSessionRepository implementations. Right?

Right. But I feel there should be a single SessionRepository bean in the app context, anything else requires some sort of compromise when it comes to consuming the repository.

So to sum things up before I make an update, we'd trim the IndexResolver contract to something like this:

public interface IndexResolver<S extends Session> {

    boolean supportsIndex(String indexName);

    String resolve(S session, String indexName);

    Map<String, String> resolveAll(S session);

}

The Delegate spec would be moved to the AbstractIndexResolver.

fitzoh added a commit to fitzoh/spring-session that referenced this pull request Aug 9, 2016

drop dependency on JdbcOperationsSessionRepository.PrincipalNameResol…
…ver by copy pasting the class. Temporary measure until spring-projects#557 is resolved.
@rwinch

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

@vpavic

I see, however I was driven by optimization considerations from the start - why run the resolver logic if we have enough knowledge to know it won't yield any result?

We don't know that we are the only repository writing to the data store. There might be something else providing the indexes. Perhaps a background job or another application.

There is a cost to maintaining this additional code with (in my opinion) little gain. In short, I still think this needs to be removed.

I really think our API should look closer to:

public interface IndexResolver<S extends Session> {

    Map<String, String> resolveIndexesFor(S session);
}

Then we can provide an abstract class that supports resolving a single index. For example:

public abstract class AbstractSingleIndexResolver<S extends Session>  implements IndexResolver<S> {
    private final String sessionIndexName;

    protected AbstractSingleIndexResolver(String sessionIndexName) {
        this.sessionIndexName = sessionIndexName;
    }

    protected abstract String resolveIndexValueFor(S session);

    public final Map<String,String> resolveIndexesFor(S session) {
        String sessionIndexValue = resolveIndexValueFor(session);
        return sessionIndexValue == null ? Collections.<String,String>emtpyMap() : Collections.singletonMap(sessionIndexName, sessionIndexValue);
    }
}

@rwinch rwinch self-assigned this Aug 15, 2016

@rwinch rwinch added this to the 1.3.0 M1 milestone Aug 15, 2016

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

We don't know that we are the only repository writing to the data store. There might be something else providing the indexes. Perhaps a background job or another application.

@rwinch Thanks for the insight, all clear now, I wasn't aware that this was an option to consider.

I can update the PR with your suggested API within few days and try to wire it altogether into repository implementations.

@rwinch

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

@vpavic Thanks!

@vpavic vpavic force-pushed the vpavic:gh-376 branch from d1da4dd to 6e543d7 Aug 17, 2016

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2016

@rwinch I've updated the PR. Please verify that the implementation is now OK with you, before I continue forward.

@rwinch

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@vpavic Sorry for the delay getting back to you. Yes this is what I had in mind. Thanks again!

@rwinch

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@vpavic I should have been more explicit. I can merge this once you are able to add testing and documentation.

Thanks again for everything you do for Spring Session and Spring in general!

@rwinch rwinch modified the milestones: 1.3.0 M1, 1.3.0 M2 Sep 7, 2016

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2016

@rwinch Thanks for the feedback!

I'll try to get back to this next week and integrate the IndexResolver into FindByIndexNameSessionRepository implementations, as well as write the tests.

@vpavic vpavic removed this from the 1.4.0 M1 milestone Jun 21, 2017

@rwinch rwinch modified the milestones: 2.0.0.M3, 2.0.0.M4 Jul 21, 2017

@rwinch rwinch modified the milestones: 2.0.0.M4, 2.0.0.M5 Sep 13, 2017

@rwinch rwinch modified the milestones: 2.0.0.M5, 2.0.0.RC1 Oct 3, 2017

@vpavic vpavic removed this from the 2.0.0.RC1 milestone Oct 4, 2017

@vpavic vpavic added this to the General Backlog milestone Sep 22, 2018

@vpavic vpavic force-pushed the vpavic:gh-376 branch from 6d97c39 to dbdfe63 Apr 30, 2019

@vpavic vpavic changed the base branch from 1.4.x to master Apr 30, 2019

@vpavic vpavic requested a review from rwinch Apr 30, 2019

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

With #1145 resolved, I've finally revisited this and rebased on master. I believe this is now inline with your feedback @rwinch - it's been a while, but maybe we can finally get this merged for 2.2.0.

@rwinch rwinch removed the Data Store label May 22, 2019

@vpavic vpavic added the in: core label Jun 6, 2019

@vpavic vpavic referenced this pull request Jun 6, 2019

Closed

Extract IndexResolver #376

@vpavic vpavic force-pushed the vpavic:gh-376 branch from dbdfe63 to 280ca95 Jun 18, 2019

@rwinch

rwinch approved these changes Jun 19, 2019

@vpavic vpavic modified the milestones: General Backlog, 2.2.0.M3 Jun 19, 2019

@vpavic vpavic self-assigned this Jun 19, 2019

@vpavic vpavic force-pushed the vpavic:gh-376 branch from 280ca95 to 568fd1e Jun 20, 2019

@vpavic vpavic closed this in a6f6042 Jun 24, 2019

@vpavic vpavic deleted the vpavic:gh-376 branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.