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

Added classes Connect.Call and StoredProcedureOutcome #53

Merged
merged 13 commits into from
Nov 22, 2016
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,8 @@ target/
.idea/
*.iml
.DS_Store
/bin/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil do we need this first slash here?

.classpath
.project
.settings/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil unnecessary newline

34 changes: 34 additions & 0 deletions src/main/java/com/jcabi/jdbc/Connect.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,52 @@ interface Connect {
*/
PreparedStatement open(Connection conn) throws SQLException;

/**
* Connect which opens a <b>CallableStatement</b>, which
* is used for calling stored procedures.
*/
final class Call implements Connect {

/**
* SQL function call.
*/
private final String sql;

/**
* Ctor.
* @param query Query
*/
public Call(final String query) {
this.sql = query;
}

@Override
public PreparedStatement open(final Connection conn)
throws SQLException {
return conn.prepareCall(this.sql);
}

}

/**
* Plain, without keys.
*/
@Immutable
final class Plain implements Connect {

/**
* SQL query.
*/
private final transient String sql;

/**
* Ctor.
* @param query Query
*/
public Plain(final String query) {
this.sql = query;
}

@Override
public PreparedStatement open(final Connection conn)
throws SQLException {
Expand All @@ -81,17 +111,20 @@ public PreparedStatement open(final Connection conn)
*/
@Immutable
final class WithKeys implements Connect {

/**
* SQL query.
*/
private final transient String sql;

/**
* Ctor.
* @param query Query
*/
public WithKeys(final String query) {
this.sql = query;
}

@Override
public PreparedStatement open(final Connection conn)
throws SQLException {
Expand All @@ -100,6 +133,7 @@ public PreparedStatement open(final Connection conn)
Statement.RETURN_GENERATED_KEYS
);
}

}

}
20 changes: 20 additions & 0 deletions src/main/java/com/jcabi/jdbc/JdbcSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
* @author Yegor Bugayenko ([email protected])
* @version $Id$
* @since 0.1.8
* @checkstyle ClassDataAbstractionCouplingCheck (500 lines)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil along with that we should add puzzle to break down this class, as it became too big

*/
@ToString
@EqualsAndHashCode(of = { "source", "connection", "args", "auto", "query" })
Expand Down Expand Up @@ -308,6 +309,25 @@ public <T> T update(final Outcome<T> outcome)
);
}

/**
* Call an SQL stored procedure.
*
* <p>JDBC connection is opened and, optionally, closed by this method.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil does this method really close connection? where?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas the run method does that in the finally block. All the other methods, e.g. execute() say that in the javadoc, so I left it there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil OK, let it be. But I still does not get optionally concept, can you explain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas Yes. In run's finally block the statement is closed, not the connection (I made a mistake in the first comment). optionally means if it's autocommit or not... "closing" the connection actually means "commiting" it. And autocommit is dictated by class boolean this.auto which can be set with the autocommit(boolean) method. The Javadoc actually has in regards the fluency of the API, that's why it seems a bit hard to understand, in my opinion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil right, so can you put the above information to Javadoc? It's more accurate than saying connection is opened and, optionally, closed

*
* @param <T> Type of result expected
* @param outcome Outcome of the operation
* @return This object
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil what does it mean? It's not just return this;

* @throws SQLException If fails
*/
public <T> T call(final Outcome<T> outcome)
throws SQLException {
return this.run(
outcome,
new Connect.Call(this.query),
Request.EXECUTE_UPDATE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil can we join these lines?

);
}

/**
* Make SQL request expecting no response from the server.
*
Expand Down
111 changes: 111 additions & 0 deletions src/main/java/com/jcabi/jdbc/StoredProcedureOutcome.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/**
* Copyright (c) 2012-2015, jcabi.com
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met: 1) Redistributions of source code must retain the above
* copyright notice, this list of conditions and the following
* disclaimer. 2) Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution. 3) Neither the name of the jcabi.com nor
* the names of its contributors may be used to endorse or promote
* products derived from this software without specific prior written
* permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
* NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
* THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
* INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
* OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.jcabi.jdbc;

import com.jcabi.aspects.Immutable;
import java.sql.CallableStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import lombok.EqualsAndHashCode;
import lombok.ToString;

/**
* Outcome of a stored procedure with OUT parameters.
* @author Mihai Andronache ([email protected])
* @version $Id$
* @since 0.7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil should be 0.17

* @param <T> Type of items
*/
@Immutable
@ToString
@EqualsAndHashCode(of = { "outpidx" })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil do we need these curly braces here?

public final class StoredProcedureOutcome<T> implements Outcome<T> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil can we have a test for this class? Other Outcome classes have them

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas is it ok of I add a todo?? Please make up ur mind about this in the beginning next time. U focus on indentation first and then u stretch my work for days...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil for me to-do is OK, however pdd is rather about making test first and puzzling implementation - otherwise we allow untested code for some time

Copy link
Member Author

@amihaiemil amihaiemil Aug 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas I tried creating a unit test, but it does not work. From what I read on SO and the official site, H2 database does not support stored procedures per se, only java functions declared as aliases: http://www.h2database.com/html/features.html#user_defined_functions

I will leave this as it is since it's already well tested in the IT; is that ok with u? The effort of finding another mock DB only for this is simply not worth it IMO.


/**
* Indexes of the OUT params.
*/
@Immutable.Array
private final transient int[] outpidx;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil why it is called outpidx? Can you explain at least in Javadoc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas it's short for "output parameter's indexes". It says in the javadoc :D

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil gosh, so cryptic :) I'd prefer just indexes if possible


/**
* Ctor.
* @param nrop Number of OUT params. Has to be > 0.
* If this ctor is used, it is assumed that the OUT parameters
* are from index 1 to including nrop.
*/
public StoredProcedureOutcome(final int nrop) {
Copy link

@mkordas mkordas Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil do you know http://www.yegor256.com/2015/01/12/compound-name-is-code-smell.html article?
nrop would be compound name here, as it is just numberOfOutParameters - while article suggests choosing just one word instead

if (nrop <= 0) {
throw new IllegalArgumentException(
"Nr of out params has to be a positive int!"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil for debugging usually it's good idea to print what was the provided argument (more context)

);
}
this.outpidx = new int[nrop];
for (int idx = 0; idx < nrop; ++idx) {
this.outpidx[idx] = idx + 1;
}
}

/**
* Ctor.
* @param opidx Indexes of the OUT params.
* <b>Index count starts from 1</b>.
*/
public StoredProcedureOutcome(final int... opidx) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil here we have cryptic param name as well

if (opidx == null || opidx.length == 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil do we really need to check for null here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas I say yes, to avoid NPE; I need to check the length since it's mandatory to have at least one output parameter. What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil in all projects we try to eliminate null as much as possible - if someone uses null and gets NPE it's their fault :)

throw new IllegalArgumentException(
"At least 1 OUT param's index needs to be specified!"
);
}
final int size = opidx.length;
this.outpidx = new int[size];
for (int idx = 0; idx < size; ++idx) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil this looks like code duplication with the above one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas I see... it really is, and it cannot be removed. Usually we call one ctor from another one using this(...) to avoid duplication, but here it is not possible since first we have to turn the int from the first ctor, in an array, and the call this(...) has to be the first.

I removed the first ctor and updated the PR description accordingly. It was in fact a convenience ctor and I figured it's not worth the duplication.

this.outpidx[idx] = opidx[idx];
}
}

@Override
@SuppressWarnings("unchecked")
public T handle(
final ResultSet rset, final Statement stmt
) throws SQLException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil paired brackets notation does not apply to method declarations

final int nrop = this.outpidx.length;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil what is nrop? can we use full name?

final Object[] outs = new Object[nrop];
if (stmt instanceof CallableStatement) {
for (int idx = 0; idx < nrop; ++idx) {
outs[idx] = ((CallableStatement) stmt).getObject(
this.outpidx[idx]
);
}
}
return (T) outs;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil is this always safe casting? T may be whatever, while casting will be successfull only if T is array of objects, right?

Copy link
Member Author

@amihaiemil amihaiemil Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas Right - I see in other Outcomes they solved the issue by requesting from the user the Class<T> object in the constructor, and later in handle it compares it to different acceped .class, like String, byte[] etc. Finally if none matches, IllegalArgumentException

In my case I would let it fail and say in the Javadoc that Object[] is needed, since asking for the Class<T> argument in the constructor seems pointless (it could only have the value Object[].class, so why make it a parameter? It should be intuitive though, since you're calling a stored procedure and my Outcome cannot know which types are there, so you should expect an Object[]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil maybe it's silly question, but why T cannot be somehow bound to Object[]? Can't it be more typesafe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas It doesn't get any better than this - I tried every combination of T and Object[] (actually this casting and generics stuff is good candidate for exam questions, you pretty much need to have a compiler in your head to know everything). Initially, I figured maybe it would simply let me return Object, but it doesn't.

}

}
83 changes: 83 additions & 0 deletions src/test/java/com/jcabi/jdbc/JdbcSessionITCase.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@

import com.jcabi.aspects.Tv;
import com.jolbox.bonecp.BoneCPDataSource;
import java.sql.CallableStatement;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Types;
import javax.sql.DataSource;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -40,6 +46,7 @@
* Integration case for {@link JdbcSession}.
* @author Yegor Bugayenko ([email protected])
* @version $Id$
* @checkstyle StringLiteralsConcatenationCheck (300 lines)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil we shouldn't be suppressing this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas there's no other way to do it: if I concatenate with StringBuilder it also complains. If I leave it in one line it's more than 80 chars.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas And I wouldn't read it from a file just for this. Would you?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
public final class JdbcSessionITCase {

Expand Down Expand Up @@ -90,6 +97,82 @@ public void changesTransactionIsolationLevel() throws Exception {
new JdbcSession(source).sql("VACUUM").execute();
}

/**
* JdbcSession can run a function (stored procedure) with
* output parameters.
* @throws Exception If something goes wrong
*/
@Test
public void callsFunctionWithOutParam() throws Exception {
final DataSource dsrc = JdbcSessionITCase.source();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil can we name it just source?

new JdbcSession(dsrc).autocommit(false).sql(
"CREATE TABLE IF NOT EXISTS users (name VARCHAR(50))"
).execute().sql("INSERT INTO users (name) VALUES (?)")
.set("Jeff Charles").execute().sql(
"CREATE OR REPLACE FUNCTION getUser(username OUT text)"
+ " AS $$ BEGIN SELECT name INTO username from users; END;"
+ " $$ LANGUAGE plpgsql;"
).execute().commit();
final Object[] result = new JdbcSession(dsrc)
.sql("{call getUser(?)}")
.prepare(
new Preparation() {
@Override
public void prepare(
final PreparedStatement stmt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil paired brackets notation does not apply here as well

) throws SQLException {
((CallableStatement) stmt)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil can we somehow avoid this cast here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas Not at the moment, since the class type tree is as follows: Statement > PreparedStatement > CallablaStatement . Maybe with a little redesign of the Connect/Preparation mechanism yes. Do you want me to add a todo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas In fact, I don't even know how to get rid of it; make a redesign and change stuff, add flow just for CallableStatement, all just to avoid a type cast...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil OK, let it be

.registerOutParameter(1, Types.VARCHAR);
}
}
)
.call(new StoredProcedureOutcome<Object[]>(1));
MatcherAssert.assertThat(result.length, Matchers.is(1));
MatcherAssert.assertThat(
result[0].toString(),
Matchers.containsString("Charles")
);
}

/**
* JdbcSession can run a function (stored procedure) with
* input and output parameters.
* @throws Exception If something goes wrong
*/
@Test
public void callsFunctionWithInOutParam() throws Exception {
final DataSource dsrc = JdbcSessionITCase.source();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil same here, just source

new JdbcSession(dsrc).autocommit(false).sql(
"CREATE TABLE IF NOT EXISTS usersid (id INTEGER, name VARCHAR(50))"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil what is usersid? did you mean userids?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas no, I meant usersid because its usernames and ids, not only ids of the users

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil so why not usersids?

).execute().sql("INSERT INTO usersid (id, name) VALUES (?, ?)")
.set(1).set("Marco Polo").execute().sql(
"CREATE OR REPLACE FUNCTION getUserById(uid IN INTEGER,"
+ " usrnm OUT text) AS $$ BEGIN"
+ " SELECT name INTO usrnm FROM usersid WHERE id=uid;"
+ " END; $$ LANGUAGE plpgsql;"
).execute().commit();
final Object[] result = new JdbcSession(dsrc)
.sql("{call getUserById(?, ?)}")
.set(1)
.prepare(
new Preparation() {
@Override
public void prepare(
final PreparedStatement stmt
) throws SQLException {
((CallableStatement) stmt)
.registerOutParameter(2, Types.VARCHAR);
}
}
)
.call(new StoredProcedureOutcome<Object[]>(new int[] {2}));
MatcherAssert.assertThat(result.length, Matchers.is(1));
MatcherAssert.assertThat(
result[0].toString(),
Matchers.containsString("Polo")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil why we check Polo instead of Marco Polo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas because then it complains of douvle literals and it is safe enough to have a contains, rahter than declare a variable for it... all the tests in all the jcabi libs are like this. Contains is used instead of equals wjere appropriate

Copy link

@mkordas mkordas Aug 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amihaiemil IMO it breaks test data coupling rule, but let's follow convention then

);
}

/**
* Get data source.
* @return Source
Expand Down