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
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@ target/
.idea/
*.iml
.DS_Store
bin/
.classpath
.project
.settings/
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@
<artifactId>validation-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<scope>test</scope>
Copy link

Choose a reason for hiding this comment

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

@amihaiemil is the verision provided in parent?

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 it is

Copy link

Choose a reason for hiding this comment

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

@amihaiemil thanks for confirming

</dependency>
<dependency>
<groupId>org.aspectj</groupId>
<artifactId>aspectjrt</artifactId>
Expand Down Expand Up @@ -428,6 +433,7 @@
</goals>
<configuration>
<repoToken>${coveralls.token.jdbc}</repoToken>
<timestampFormat>yyyy-MM-dd'T'HH:mm:ss</timestampFormat>
</configuration>
</execution>
</executions>
Expand Down
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
);
}

}

}
24 changes: 24 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,10 @@
* @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

* @todo #51:30min Refactor this class to avoid too much coupling.
* For instance, CRUD operations could be performed by another class.
Copy link

Choose a reason for hiding this comment

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

@amihaiemil can you also add remnidner to remove suppression?

* Don't forget to remove the suppressions that become obsolete afterwards.
*/
@ToString
@EqualsAndHashCode(of = { "source", "connection", "args", "auto", "query" })
Expand Down Expand Up @@ -308,6 +312,26 @@ public <T> T update(final Outcome<T> outcome)
);
}

/**
* Call an SQL stored procedure.
*
* <p>JDBC connection is opened and, optionally, commited by this
* method, depending on the <b>autocommit</b> class attribute:
* if it's value is true, the connection will be commited after
* this call.
*
* @param <T> Type of result expected
* @param outcome Outcome of the operation
* @return Result of type T
* @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
);
}

/**
* Make SQL request expecting no response from the server.
*
Expand Down
92 changes: 92 additions & 0 deletions src/main/java/com/jcabi/jdbc/StoredProcedureOutcome.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* 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.17
* @param <T> Type of the returned result, which <b>has to be</b> Object[]
*/
@Immutable
@ToString
@EqualsAndHashCode
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.


/**
* OUT parameters' indexes.
*/
@Immutable.Array
private final transient int[] indexes;

/**
* Ctor.
* @param indexes Indexes of the OUT params.
* <b>Index count starts from 1</b>.
*/
public StoredProcedureOutcome(final int... indexes) {
if (indexes.length == 0) {
throw new IllegalArgumentException(
"At least 1 OUT param's index needs to be specified!"
);
}
final int size = indexes.length;
this.indexes = 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.indexes[idx] = indexes[idx];
}
}

@Override
@SuppressWarnings("unchecked")
public T handle(final ResultSet rset, final Statement stmt)
throws SQLException {
final int params = this.indexes.length;
final Object[] outs = new Object[params];
if (stmt instanceof CallableStatement) {
for (int idx = 0; idx < params; ++idx) {
outs[idx] = ((CallableStatement) stmt).getObject(
this.indexes[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.

}

}
95 changes: 95 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,15 @@

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 java.util.Date;
import javax.sql.DataSource;
import org.apache.commons.lang3.StringUtils;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -90,6 +98,93 @@ 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 source = JdbcSessionITCase.source();
new JdbcSession(source).autocommit(false).sql(
"CREATE TABLE IF NOT EXISTS users (name VARCHAR(50))"
).execute().sql("INSERT INTO users (name) VALUES (?)")
.set("Jeff Charles").execute().sql(
StringUtils.join(
"CREATE OR REPLACE FUNCTION fetchUser(username OUT text,",
Copy link

Choose a reason for hiding this comment

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

@amihaiemil is it on purpose that here you use username and in the test below usrnm?

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, since they are SQL views which hold the result of each SELECT, and they will exist at the same time; I didn't want to "step on each other's toes"

" day OUT date)",
" AS $$ BEGIN SELECT name, CURRENT_DATE INTO username, day",
" FROM users; END; $$ LANGUAGE plpgsql;"
)
).execute().commit();
final Object[] result = new JdbcSession(source)
.sql("{call fetchUser(?, ?)}")
.prepare(
new Preparation() {
@Override
public void
prepare(final PreparedStatement stmt)
throws SQLException {
final CallableStatement cstmt =
(CallableStatement) stmt;
cstmt.registerOutParameter(1, Types.VARCHAR);
cstmt.registerOutParameter(2, Types.DATE);
}
}
)
.call(new StoredProcedureOutcome<Object[]>(1, 2));
MatcherAssert.assertThat(result.length, Matchers.is(2));
MatcherAssert.assertThat(
result[0].toString(),
Matchers.containsString("Charles")
);
MatcherAssert.assertThat(
(Date) result[1],
Matchers.notNullValue()
);
}

/**
* 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 source = JdbcSessionITCase.source();
new JdbcSession(source).autocommit(false).sql(
"CREATE TABLE IF NOT EXISTS usersids (id INTEGER, name VARCHAR(50))"
).execute().sql("INSERT INTO usersids (id, name) VALUES (?, ?)")
.set(1).set("Marco Polo").execute().sql(
StringUtils.join(
"CREATE OR REPLACE FUNCTION fetchUserById(uid IN INTEGER,",
" usrnm OUT text) AS $$ BEGIN",
Copy link

Choose a reason for hiding this comment

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

@amihaiemil probably I'm not educated enough, but what's $$ 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 To be honest, I'm not sure. It's the first time I work with PSQL and it's the same in all (I mean ALL) the stored procedure examples I could find. Search for "postgresql $$ operator" doesn't return anything either. Do you know it works without it? Even if it works, I assume there must be a reason for it. It's also there in the official examples here

Copy link
Member Author

@amihaiemil amihaiemil Aug 17, 2016

Choose a reason for hiding this comment

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

@mkordas found the explanation in this thread. Turns out they replace the single quotes (which are necessary), to avoit quotation/escaping issues

Copy link

Choose a reason for hiding this comment

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

@amihaiemil thanks for figuring it out, maybe we could drop a very short note about their official name (so that they become googleable) in Javadoc or comment?

Copy link

Choose a reason for hiding this comment

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

@amihaiemil what about this one?

" SELECT name INTO usrnm FROM usersids WHERE id=uid;",
" END; $$ LANGUAGE plpgsql;"
)
).execute().commit();
final Object[] result = new JdbcSession(source)
.sql("{call fetchUserById(?, ?)}")
.set(1)
.prepare(
new Preparation() {
@Override
public void
prepare(final PreparedStatement stmt)
throws SQLException {
((CallableStatement) stmt)
.registerOutParameter(2, Types.VARCHAR);
}
}
)
.call(new StoredProcedureOutcome<Object[]>(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