-
Notifications
You must be signed in to change notification settings - Fork 231
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
Proposed new API design principles #83
Comments
So let's start...
|
|
|
|
I agree
I'm not sure I agree that it is awful! :). But I agree that it's better to use patterns under the GoF umbrella.
I think this is a really good idea.
As above. It think this is a really good idea :) |
I think your vision for sql2o v2 is very promising. I have read though your comments, and I think it looks very promising! Some things I think is important:
I think all of this fits nicely into your vision! Something I would like to discuss:
|
Yes, checked exception is wrost Java feature even. But re-throwing all of them as |
I think the current design for sql2o exceptions is good and shouldn't On Thursday, April 10, 2014, Dmitry notifications@github.com wrote:
|
I think better way is interface ResultSetExtractor{
Object extractValue(QueryResult resultSet, int columnNumber) throws SQLException;
} back direction is current converters become Extractor|Setter factories |
WARNING! IT'S IMHO |
great idea |
this is a subject to think about. this is not so easy, think about distributed transactions... // using is 100% same as try-with-resource
using(Transaction tr = new Transaction()){
// any code here automatically enlisted in transaction
// this is done via ThreadLocal<Stack<Transaction>>
// every constructor invocation puts new transaction onto stack top
// every dispose() (dispose == java AutoClosable.close() )
// removes transaction from stack
// every database statement constructor checks top of this stack
// if it's not empty it will enlist in transaction
this.doSomething();
tr.commit();
} |
this is subject to talk about. I think (we can discuss and also look for best practics) wrapping everything in runtime exceptioon is evil (look my explanation in previous post). It's better to teach 'users' better and clean programming style (wich will help in other cases to, not just sql2o) |
That is roughly how I use sql2o in my projects right now. I have a data To be honest I don't think sql2o should implement this, because it's Maybe someone could write an extension that keeps a stack of transactions On Thu, Apr 10, 2014 at 6:03 PM, Dmitry notifications@github.com wrote:
|
Transactions are main DB feature. We must provide transaction management some way... |
Yes I agree, I'm just saying that IMO we don't need to provide anything Small projects do not need large data frameworks. Large projects will have Again this is just my opinion, but the minimalism of sql2o is what On Thu, Apr 10, 2014 at 6:22 PM, Dmitry notifications@github.com wrote:
|
This was the original idea... I programmed the first version in a couple of nights, as a simple proof of concept, and showed it to some colleagues. They immediately started using it! The goal of sql2o has never been to enforce best practices or to learn people to use property programming techniques. It was to make something that is extremely easy and intuitive to use! |
Here is what I think would work. Starting point Connection Query I think the above is a good starting point. But I would like keep many of the convenience-methods sql2o have today.
The above is in many ways the identity of sql2o. This should also be available in v 2. It is a convenient way of opening a connection, executing a query and mapping it to pojo objects. But I also think something like following should be possible:
or if you just need to run something in a transaction
I guess the key is to make sure it has has a flexible model under the hood, and still keep convenience-functionality for the things we do all the time. |
since this is just facade methods it's ok. but I really dislike this let me figure out why. this.list1 = sql2o.createQuery(sql)
.addParameter('param', val)
.executeAndFetch(Pojo.class); now you need to add list2 with Pojo2 obtained by sql2. Now suggest what junior will write ;) this.list1 = sql2o.createQuery(sql)
.addParameter('param', val)
.executeAndFetch(Pojo.class);
this.list2 = sql2o.createQuery(sql2)
.addParameter('param', val)
.executeAndFetch(Pojo2.class); |
I've been thinking, and here is what I propose. Sql2o objectFirst, I would like to pull back my earlier comment that it should be possible to run queries directly on the sql2o object. return sql2o.createQuery("select ...")
.addParameter("param1", val)
.addParameter("param2", val)
.executeAndFetch(Pojo.class); This approach is very convenient, as long as you only want to run 1 query.
So, after some consideration, I support removing the createQuery function from the sql2o object. ConnectionsConnection should be opened using try-with-resource feature try(Connection connection = sql2o.open()) {
...
} queries are created from connection object. List<Pojo> pojos = connection.createQuery("select * from ...")
.addParameters(...)
.executeAndFetch(Pojo.class); QueriesAs @dimzon has already proposed (And implemented :), Query class should also be autoClosable, so if you execute many queries on the same connection, you can close queries and their underlaying statements as soon as they have been executed. try(Query query = connection.createQuery("select * from ...")) {
List<Pojo> pojos = query.addParameters(...).executeAndFetch(Pojo.class);
} As it is generally good practice to close connections as fast as possible, and the above syntax adds complexity, it should not be required to close statements. Sql2o should clean up automatically, and call close on all statements created with a connection, when the connection is closed. TransactionsTransactions are opened on a connection. They should also be autoclosable, so we have a clear scope for the transaction try(Connection connection = sql2o.open()) {
// execute a query not using a transaction.
List<Pojo> pojos = connection.createQuery("select * from ...")
.addParameters(...)
.executeAndFetch(Pojo.class);
// run some updates in a transaction.
try (Transaction transaction = connection.beginTransaction()) {
conection.createQuery("insert into ....")
.addParameters("..", val1)
.addParameters("..", val2)
.executeUpdate();
conection.createQuery("insert into ....")
.addParameters("..", val1)
.addParameters("..", val2)
.executeUpdate();
transaction.commit();
}
// queries run on the connection after the transaction is closed, will be autocommitted.
} I also think we should supply a java 8 lambda syntax for the transactions, similar to how i works on todays version. try(final Connection connection = sql2o.open()) {
// execute a query not using a transaction.
List<Pojo> pojos = connection.createQuery("select * from ...")
.addParameters(...)
.executeAndFetch(Pojo.class);
// run some updates in a transaction.
connection.runInTransaction(() -> { // or should we rename this to withTransaction() ?
connection.createQuery("insert ..", val1).executeUpdate();
connection.createQuery("insert ..", val2).executeUpdate();
// if everything when ok (No exceptions), transaction is committed automatically.
// If an unhandled exception is thrown, transaction is rolled back.
});
// or if you need to have some more control over the transaction.
connection.runInTransaction((Transaction t) -> {
connection.createQuery("insert ..", val1).executeUpdate();
connection.createQuery("insert ..", val2).executeUpdate();
if (somethingIsWrong) {
t.rollback();
} else {
// we don't need to explicitely commit transaction,
// as that is done automatically.
}
});
} And if we need to add transaction savepoints: try(final Connection connection = sql2o.open()) {
// execute a query not using a transaction.
List<Pojo> pojos = connection.createQuery("select * from ...")
.addParameters(...)
.executeAndFetch(Pojo.class);
// or if you need to have some more control over the transaction.
connection.runInTransaction((Transaction t) -> {
SavePoint save1 = t.setSavePoint();
connection.createQuery("insert ..", val1).executeUpdate();
SavePoint save2 = t.setSavePoint();
connection.createQuery("insert ..", val2).executeUpdate();
if (somethingIsWrong) {
t.rollback(save2);
} else if (everythingWentWrong) {
t.rollback(save1);
} else {
// we don't need to explicitely commit transaction,
// as that is done automatically.
}
});
} Java 7We will base much of the api on java 7 autoClosable feature, so I also support changing target to java 7. Also......I think @dimzon brought up an excellent point when he suggested that it should be possible to create an object from the corresponding JDBC object it extends. So a sql2o Connection can be created from a JDBC connection and sql2o Query object can be created from a PreparedStatement. try(Query q = new Query(statement)) {
List<Pojo> l = q.executeAndFetch(...);
} And we should expose the PojoMapper, so it can be used directly with a ResultSet. Also, pojo mapping should be implemented using a strategy pattern, so users can change pojo mapping strategy. In this way we can supply a default PojoMappingStrategy that is roughly like the one we use today. But users could supply their own PojoMapper that, for instance, uses JPA-annotations or similiar. Sql2o version 2I had planned to take the changes we have already made, and publish a version 1.5. I think I'll skip that, so we can focus on version 2 from now on. @dimzon, I have added you as collaborator, so you can assign yourself to issues, close issues, create branches in main sql2o project and so forth. So... any objections, or is this something we can agree on? ~lars |
I also propose to re-visit some method names like |
+1 for the naming idea, I usually associated "execute" with a write Also I didn't respond to Lars' proposal, but everything looks good to me! I On Friday, April 25, 2014, Dmitry notifications@github.com wrote:
|
talking about thread-safety - it's important for singleton objects (like Sql2o, Convert, FeatureDetector). It's not necessary for other objects. there are difference between transaction itself and checkpoint within transacton - it's impossible to set different isolation level for checkpoint... So for clearance I propose @aaberg |
commit vs rollback - what is "default" behavior?
|
Tranactions@aldenquimby I think your suggestion of tracking transactions, and only committing the outermost one makes sense. @dimzon I believe you are right, that "rollback" should be the default action. Method renamingWhat about this? I think we should deprecate and keep the old method names as well. At least for a while. This makes it easier to upgrade from an earlier version of sql2o. |
I also have a couple of suggestions: Andrea |
Hi, Antonio |
@jammer76 does your java classes have public fields or getters/setters? sql2o use bytecode generators for methods (really, they reuse them from JVM). |
Hi Dimzon, |
Try public fields
|
But public fields is not a good choice... for sure is faster but absolutely unsafe and unmaintanable... ;-) |
The bottleneck is not reflection since byte code used. Just one more
|
It depends. Until they are package private...
|
Btw I think we need a strategy to ignore setters and deserualize directly
|
Such a strategy should be optional and should not be enabled by default. it will be counter intuitive compared to ordinary java-standards. But I agree, that such a strategy could be nice to have, when you need a little extra performance for large datasets. |
Btw GSON use such strategy by default. Java builtin serialization too
|
Since serialization means persisting object state and actual state is
|
So saving field means "Save object snapshot" and using properties means
|
Yes, it really depends on the use case. I think the best solution would be to have both strategies available, and to keep the properties-first strategy as default. |
For Serialization Strategy coul be useful to investigate also KRYO (faster that I know) |
As I said before bottleneck is additional virtual methods into call stack
|
@dimzon GSON makes use of sun.misc.unsafe to handle serialization/deserialization using private fields. It's definitely a nice feature to have as your model objects do not require setters (thus making them mutable everywhere), but I would be worried about sun.misc.unsafe being removed in future java versions. |
It's impossible to completely take away such mechanism since Java RMI
|
I want to propose to re-design new (2.0) release version architecture|api using following principles
The text was updated successfully, but these errors were encountered: