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

Query interception support #334

Closed
miere opened this issue Apr 28, 2016 · 27 comments
Closed

Query interception support #334

miere opened this issue Apr 28, 2016 · 27 comments

Comments

@miere
Copy link

miere commented Apr 28, 2016

PS: This is a copied and pasted text from an earlier discussion about supporting query interception at the #134 issue

Hi guys, I would like to suggest a different approach at this discussion and I'd like to hear your honest feedback about it...

I was reading this topic before came here. And, I agree with you guys that support cache layers are totally application dependent. But, I would like to set a point here: support cache layers would be hard to maintain in the long term, but allow developers to cache its queries isn't that hard.

Let's imagine that, instead of providing a cache abstraction, JDBI could provide a way to "intercept" queries. Bellow is my suggestion:

// Here an interface that could be provided by JDBI core module.
public interface QueryInterceptor {
  // ExecutableMethod could be a wrapper to the java.lang.reflect.Method and the DAO implementation
  // Another approach to this method would be store the arguments and the query into the ExecutableMethod too...
  Object interceptQuery( ExecutableMethod daoMethod, String query, Object[] args );
}

// at some place on your code, you can attach the query interceptor into the Handler
handler.attachQueryInterceptor( new MyQueryInterceptor() );

// Here a valid implementation that does nothing but call the query
public class MyQueryInterceptor {

  public Object interceptQuery( ExecutableMethod daoMethod, String query, Object[] args ) {
    return daoMethod.invoke( args );
  }
}

// Developers whom intend to cache their queries could create their own algorithm
// Bellow a non-thread-safe sample code just to make my point
public class CachedQueries {

  final Map<CacheEntry, List<Object> cachedData = new HashMap<>();

  public Object interceptQuery( ExecutableMethod daoMethod, String query, Object[] args ) {
    final CacheEntry entry = new CacheEntry( query, args );

    Object resultObject = cachedData.get( entry );
    if ( resultObject == null ) {
      resultObject = daoMethod.invoke( args );
      cachedData.put( entry, resultObject );
    }

    return resultObject;
  }
}

// represents a cached entry
class CacheEntry {
  final String query;
  final Object[] args;

  // Implement here a constructor
  // Implement here a valid equals and hashCode methods
}

Of course, there is a huge room for improvement on my suggested design, but I just want to make a point: is it simple enough to be included on JDBI?

Regards

@stevenschlansker
Copy link
Member

@miere I believe this could be workable. Especially with the new extensions API, we are happy to support projects (such as a cache) that exist outside of core. If the caching layer becomes stable and popular enough we could then consider promoting it into the main build.

I agree that targeting the Query API makes more sense -- SqlObject will turn into the query API behind the scenes, so if you target the core API you kill two birds with one stone.

Maybe the first step is to take a look at the brand new extension API, and the query API, and determine exactly what hooks are necessary to allow the implementation? We could then have a PR adding the needed hooks, and you can develop your caching solution out of repo. Sound like a plan?

@miere
Copy link
Author

miere commented Apr 28, 2016

@stevenschlansker
It sounds like a plan for me. Although,I not quite sure if I'm aware about the Query API.
Do you mind to send the documentation link, or an unit test, which I can become more familiar with it?
Maybe, I just does not remember what the term means on JDBI.

@miere
Copy link
Author

miere commented Apr 28, 2016

@stevenschlansker I've removed my comment from the older issue. We can continue from here... ;)

I guess it's referred to as the 'Fluent API' elsewhere -- it's the first example on http://jdbi.org/
Nice. Thanks!

So, do you think the best approach would be using the Fluent API?
Could you, please, exemplify what do you have on your mind?

@stevenschlansker
Copy link
Member

@miere Same as your original idea, except that instead of building on top of SqlObject methods, intercept at the lower fluent / query API level. Then anyone who uses either API can take advantage of your caching.

@miere
Copy link
Author

miere commented Jun 14, 2016

Sorry for my late answer, @stevenschlansker

Especially with the new extensions API, we are happy to support projects (such as a cache) that exist outside of core.

Could please send me an example of how to use these new extensions? I'm planning to write the first lines of code regarding to this issue soon... I hope you don't mind, but I would be happy to contribute...

@chscorpio
Copy link

I also like the idea of a QueryInterceptor.

Could the solution be implemented in a manner similar to StatementCustomizers where we update SQLStatement.internalExecute(QueryResultsMunger) to call registered QueryInterceptor's just after rewriting the sql but before creating the PreparedStatement?

The QueryInterceptor interface would have to change to just receive a BaseStatement and/or a StatementContext or derived classes and then specify a return type that could be used to skip the PreparedStatement code. The return type is trickier as we still want the QueryResultsMunger to munge results.

What do you think? Is there a better approach?

@stevenschlansker
Copy link
Member

stevenschlansker commented Jul 6, 2016

@miere Take a look at the JdbiPlugin interface. It allows you to hook into various object creation within JDBI and register additional customizers. We would probably have you register one of those, and as @chscorpio suggests we could then add hooks into internalExecute and other necessary places. Unfortunately I don't know specifically which hooks we will end up needing, nor what the best way to implement them is -- someone would probably have to start prototyping the code out and we will happily discuss any needed extension points.

We are working on proper documentation, unfortunately progress is a little slow since all the core developers have day jobs :) but we're getting there! Until then the code is the only reference, sadly. There's a couple of simple plugins already GuavaPlugin, JodaTimePlugin, etc - but this would be the most advanced plugin yet, so I expect there to be some back and forth.

@miere
Copy link
Author

miere commented Jul 7, 2016

Glad to hear you back, @stevenschlansker!

Don't worry dude... I'll sure take a deep look into the JodaTimePlugin and GuavaPlugin to get some inspiration. I know how hard it is to create a community and contribute to OpenSource software having day jobs... On the next couple weeks I'll release the next kikaha's stable version, after that I'll be glad to help you guys with the query interceptor support. ;)

@stevenschlansker
Copy link
Member

Awesome, looking forward to it!

@qualidafial qualidafial added this to the JDBI 3 Release blocker milestone Jul 7, 2016
@miere
Copy link
Author

miere commented Aug 16, 2016

Hi, @chscorpio, @stevenschlansker I'm back and ready to help!

Before start to write the first lines of code regarding this issue I've spent some time to become familiar with the Jdbi3 API. At first glance: It is stable and look consistent than its previous version. Congrats!

That said, I also took a deep look into the SqlStatement.internalExecute, how does it works and how the StatementCustomizers also works. After reading the code SqlStatement::internalExecute some doubts raised on mind and I would like to talk with guys about theses a lil' bit.

Long story short:

  • How would I handle multiple QueryInterceptor per query execution? Chain Of Responsibility?
  • How could I distinguish a query that would be candidate to be cached from another one that would not, once I have no meta data associated to the current statement?

TL; DR;

I don't know what you guys think about but, before your suggestion, I had something like the Filter implementation from Servlet API for the Jdbi. I was wondering to create a class where I could be notified of any query execution.

After taking a deep look into the SqlStatement class I could realise what you guy meant with "I agree that targeting the Query API makes more sense", once everything really becomes a JDBC Statement. Considering this, I think this method would hold two arguments:

  • The first object would hold relevant meta-data I could analyse under the interceptor - lets call it ExecutionContext for now
  • The second would be the statement itself

Also I was considering:

  • The ExecutionContext would be empty if the user have used the Fluent/Query API instead of a SQLObject
  • SQLObject would instead make the QueryInterceptor to receive an ExecutionContext which developers could retrieve Annotations, of any other relevant meta data from the method that have generated this Statement
  • The second argument would use the Chain of Responsibility Pattern, if is expected to allow developers to have more than one QueryInterceptor per execution

I'm using JDBI today in a project that handle thousands of thousands of queries per second to store some statistical data. At that project there is a few queries that compute some aggregations and these aggregations would take several minutes to conclude. I was wondering to cache only these queries, once the other one took less than 10ms.

If I was able to Annotate a method from my SQL Object I would be able to identify which methods I would cache (or not) through the interceptor.

Please note this was my first though, I'm not trying to push my code design as the definitive solution here. It's quite the opposite actually: I just want to clearly understand what you guys have exactly in your mind to ensure my contribution will be useful to Jdbi at all, not only my personal needs (cache stuffs).

Cheers! \o

@qualidafial
Copy link
Member

qualidafial commented Aug 16, 2016

Hi Miere,

I've given this some thought too in the intervening months. I'm actually
coming back to thinking this would be something that would be cleaner to
wire in with SQL Objects rather than the fluent API.

As part of the work on making @Transaction work on any SQL Object method (
#411), we introduced a
@SqlMethodDecoratingAnnotation meta-annotation (alongside the existing
@SqlMethodAnnotation). Decorators may add or replace SQL method behavior.
In the case of @transaction, it just opens a transaction around the method
call.

It's not public API at the moment, but it could be made public with a
little work. In particular, there's nothing yet in the sql method decorator
handling that determines in what order decorators should be applied. This
was discussed on the pull request but it wasn't a problem that needed
solving until we had multiple decorators.

For caching, you could define a decorating annotation e.g. @Cache (possibly
with other complementary annotations like @Cachekey for arguments).

I'm thinking that a guava-specific cache implementation would be a good
backing cache to start with. We already have a jdbi3-guava artifact that
would be a good place to keep it.

Let's open an issue on Github and we can discuss it further there.

@stevenschlansker
Copy link
Member

You are talking about it on an issue on GitHub already ;)

Thanks for diving so deeply in to understand! The reason I was pushing towards implementing this via the lower level statement API is that SqlObject still cannot cover 100% of use cases (nor is it really intended to) and there are still places where the fluent api is cleaner -- if the caching artifact is built on top of SqlObject then there is a disconnect about what is potentially cacheable.

I think your idea of a Filter-like API with a context and statement handed to it could make sense. What does your ExecutionContext need that isn't already on StatementContext? Are they the same thing, or should they be different?

@qualidafial
Copy link
Member

You are talking about it on an issue on GitHub already ;)

Heh. I thought I was responding to the mailing list.. cone of shame

@miere
Copy link
Author

miere commented Aug 17, 2016

Heh. I thought I was responding to the mailing list.. cone of shame

Hehehehehe...

@stevenschlansker I've forget to consider StatementContext. If we would have access to meta-data found at the SQL Object, we can use it. But, it took a deep look into the @qualidafial suggestion:

As part of the work on making @transaction work on any SQL Object method (
#411), we introduced a
@SqlMethodDecoratingAnnotation meta-annotation (alongside the existing
@SqlMethodAnnotation). Decorators may add or replace SQL method behavior.
In the case of @transaction, it just opens a transaction around the method
call.

I was able to create a (draft of a) distributed cache here using this approach. It was very simple indeed. I see that the #411 came two months latter than this issue and it shows me how intense has been the work on Jdbi3 since then.

So, think now that it is up to you guys: both approach is quite good to me. The QueryAPI approach has the advantage to be a central point of maintenance but the @qualidafial suggestion seems more cleaner. Also, if the SqlMethodDecoratingAnnotation be your choice, I do believe we already have a Query Interceptor support (but with another name :P), thus this issue would be closed.

@qualidafial
Copy link
Member

I think the other thing is that if you're talking to the fluent API, you can just imperatively talk to a cache too.

So in my mind, it makes more sense to focus on the SqlObject layer, and try to come up with a set of declarative annotations that cover the most common use cases, and try to structure it in a way that imperative (i.e. default or non-abstract) methods in a SqlObject have the same access to caches that declarative methods do.

@qualidafial
Copy link
Member

Opened #455 as a first step to enabling this.

@miere
Copy link
Author

miere commented Nov 9, 2016

Hi guys, I'm back! Sorry I leave this issue alone for a while...

I do believe we already have a Query Interceptor support (but with another name :P)

The original intention behind this issue was provide a way to intercept queries - or SQL Objects' methods. Since the PR #455 was merged I was able to create a decorator and intercept methods. It worked perfectly on both JDBI3 versions (alpha1 and alpha2).

So in my mind, it makes more sense to focus on the SqlObject layer, and try to come up with a set of declarative annotations that cover the most common use cases, and try to structure it in a way that imperative (i.e. default or non-abstract) methods in a SqlObject have the same access to caches that declarative methods do.

I do think the same way, @qualidafial. I've spent a little time today creating a very tiny layer for Query Caching using Annotations, Decorators and SQL Objects... I didn't opened a PR for this yet, I just created the code on a sample project I have on the GitHub. I would like you guys take a deep look into the source code I've made and send me a (honest) feedback about it.

The Cache Support source code could be found here. To sum up, this package contains a Decorator that looks for CacheMechanisms available on classpath - via SPI. Also, this package contains an Annotation that could be placed on SQL Objects' methods to indicate its result should be cached for further usage. Here you can see the a very simple CacheMechanism implementation that saves the result of a method into a HashMap.

Here you can see an Integration Test with the caching mechanism in action. The query SELECT NOW(), reference by the Queries.returnTheSolutionForTheUniverseAndEverythingElse ( :P ), will be executed only once.

Please, send me feedback... okay? If you think this would be useful to JDBI3 I'll a PR tomorrow with this core implementation. I would also create a CacheMechanism for Guava into the existing jdbi3-guava module.

@qualidafial
Copy link
Member

qualidafial commented Nov 9, 2016

Hi Miere, this looks like a good start!

I'm currently advocating for a change in core that will probably alter v3's Handler interface, which would force you to do a little rework. I'm hopeful that the tradeoff will be worth the effort though--it will give users and library authors the ability to extend JDBI with their own configuration and have it be usable by anybody. I could see this [Edit: forgot to finish sentence] I could see this being a useful way of e.g. registering cache mechanisms.

I'm hesitant to adopt this caching into JDBI proper at this stage.

First off, this needs time to mature, and prove itself to work well for a variety of users and environments. If we did add it, I would probably keep the guava cache implementation separate from the jdbi3-guava.

Secondly, introducing caching is a bit of a philosophical departure (as I see it) for the project. JDBI has always acted like an interpreter between the java runtime and the database--merely translating commands and requests from one format into another without changing any semantics. Adding caching would make JDBI act as an agent (as Hibernate does) rather than an interpreter.

I personally view the session cache as Hibernate's greatest weakness--once you open this door, it becomes very easy for the library to start getting cute with you, trying to be smart on your behalf instead of just doing what you told it to. JDBI has so far avoided falling into this trap, and I'm hesitant to start treading near it now.

Please consider rereading my list of questions from the original mailing list thread for specifics. If you can come up with an API that answers those questions in a cache-implementation-neutral way, that would go a long way toward easing my concerns.

@qualidafial
Copy link
Member

@miere Please take a look at the latest code in jdbi3 branch.

JDBI 3 now provides a configuration mechanism, and you can add your own arbitrary config data by implementing JdbiConfig:

public class Caching implements JdbiConfig<Caching> {
  public Caching() {}

  private Caching(Caching that) {
    this.cacheMechanism = that.cacheMechanism;
  }

  private CacheMechanism cacheMechanism;

  public CacheMechanism getCacheMechanism() {
    return cacheMechanism;
  }

  public void setCacheMechanism(CacheMechanism cacheMechanism) {
    this.cacheMechanism = cacheMechanism;
  }

  // and any other universal caching configuration

  public Caching createCopy() {
    return new Caching(this);
  }
}

Then, in your cache method handler:

@RequiredArgsConstructor
public class CachedMethodHandler implements Handler {

  final Handler base;

  @Override
  public Object invoke(Object target,
                       Method method,
                       Object[] args,
                       HandleSupplier handle) throws Exception {
    CacheMechanism cacheMechanism = handle.getConfig(Caching.class).getCacheMechanism();

    final SQLObjectMethod sqlObjectMethod = new SQLObjectMethod(method, args);
    Object foundData = cacheMechanism.loadDataFor(sqlObjectMethod);
    if ( foundData == null ) {
      foundData = base.invoke( target, method, args, config, handle );
      cacheMechanism.storeDataFor( sqlObjectMethod, foundData );
    }
    return foundData;
  }
}

I'm planning to release another v3 alpha to Maven Central with the config stuff later today.

@miere
Copy link
Author

miere commented Nov 28, 2016

Quite nice improvement. I'll forward to this tonight.

About the Caching implementation of JdbiConfig. How I'm supposed to notify the Jdbi that there is a new JdbiConfig available? Should I register it on the Plugin configuration? Or should I provide it as SPI implementation of JdbiConfig interface?

@qualidafial
Copy link
Member

qualidafial commented Nov 28, 2016

JDBI will instantiate your config object the first time you request it for that type. Jdbi, Handle, and SqlStatement all implement Configurable interface with several convenience methods for getting at configuration.

Jdbi jdbi = ...
jdbi.getConfig(Caching.class).setCacheMechanism(new LocalCacheMechanism());

@qualidafial
Copy link
Member

I didn't get around to releasing last night--did it just a minute ago. It should be available in Central as version 3.0.0-alpha4 shortly.

@qualidafial
Copy link
Member

@miere Have you tried out your caching approach with a recent build, using the config stuff? We just released 3.0.0-alpha10 today. Beta is probably next.

@miere
Copy link
Author

miere commented Jan 26, 2017

@qualidafial thanks for asking...

Actually, I wasn't able to give a chance for alpha4 as you've mentioned before... I've been a lil' busy this last few months. I'll take a deep look into it this weekend and then I give you a feedback... Okay?

Regards

@qualidafial
Copy link
Member

Heads up, we released beta this morning

@miere
Copy link
Author

miere commented Feb 13, 2017

Hi @qualidafial I just adapted my former cache code to the 3.0.0-beta0 structure and it... worked greatly!
I had identified that all meta-information that is immutable, such as the method, are stored on the HandleSupplier object... quite nice job dude.

@qualidafial
Copy link
Member

This issue has been open for a while. The conversation has been wide-ranging, but I believe we've satisfied the core need at this point.

I'm closing this ticket. If there's any further scope you want to explore, let's discuss it in a separate issue.

Thanks all for the great discussion and feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants