Skip to content

Commit 728226e

Browse files
olim7tolim7t
committed
Merge branch '2.1' into 2.2
2 parents b52bd32 + 610c81e commit 728226e

5 files changed

Lines changed: 189 additions & 29 deletions

File tree

‎changelog/README.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@
99

1010
Merged from 2.1 branch:
1111

12+
- [bug] Special case check for 'null' string in index_options column (JAVA-834)
13+
- [improvement] Allow accessor methods with less parameters in case
14+
named bind markers are repeated (JAVA-835)
1215
- [improvement] Improve QueryBuilder API for SELECT DISTINCT (JAVA-475)
1316
- [improvement] Make NativeColumnType a top-level class (JAVA-715)
1417
- [improvement] Expose ProtocolVersion#toInt (JAVA-700)
@@ -67,6 +70,13 @@ Merged from 2.1 branch:
6770
- [improvement] Unify "Target" enum for schema elements (JAVA-782)
6871

6972

73+
### 2.1.7.1
74+
75+
- [bug] Special case check for 'null' string in index_options column (JAVA-834)
76+
- [improvement] Allow accessor methods with less parameters in case
77+
named bind markers are repeated (JAVA-835)
78+
79+
7080
### 2.1.7
7181

7282
- [improvement] Improve QueryBuilder API for SELECT DISTINCT (JAVA-475)

‎driver-core/src/main/java/com/datastax/driver/core/ColumnMetadata.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -277,11 +277,16 @@ private static IndexMetadata build(ColumnMetadata column, Map<String, String> in
277277
if (type == null)
278278
return null;
279279

280-
if (!indexColumns.containsKey(INDEX_OPTIONS))
280+
// Special case check for the value of the index_options column being a string with value 'null' as this
281+
// column appears to be set this way (JAVA-834).
282+
String indexOptionsCol = indexColumns.get(INDEX_OPTIONS);
283+
if (indexOptionsCol == null || indexOptionsCol.isEmpty() || indexOptionsCol.equals("null"))
281284
return new IndexMetadata(column, indexColumns.get(INDEX_NAME));
285+
else {
286+
Map<String, String> indexOptions = SimpleJSONParser.parseStringMap(indexOptionsCol);
287+
return new IndexMetadata(column, indexColumns.get(INDEX_NAME), indexOptions);
288+
}
282289

283-
Map<String, String> indexOptions = SimpleJSONParser.parseStringMap(indexColumns.get(INDEX_OPTIONS));
284-
return new IndexMetadata(column, indexColumns.get(INDEX_NAME), indexOptions);
285290
}
286291
}
287292

‎driver-core/src/test/java/com/datastax/driver/core/IndexMetadataTest.java‎

Lines changed: 51 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,19 @@
4343
major = 1.2)
4444
public class IndexMetadataTest extends CCMBridge.PerClassSingleNodeCluster {
4545

46+
/**
47+
* Column definitions for schema_columns table.
48+
*/
49+
private static final ColumnDefinitions defs = new ColumnDefinitions(new ColumnDefinitions.Definition[] {
50+
definition(COLUMN_NAME, text()),
51+
definition(COMPONENT_INDEX, cint()),
52+
definition(KIND, text()),
53+
definition(INDEX_NAME, text()),
54+
definition(INDEX_TYPE, text()),
55+
definition(VALIDATOR, text()),
56+
definition(INDEX_OPTIONS, text())
57+
});
58+
4659
@Override
4760
protected Collection<String> getTableDefinitions() {
4861
String createTable = "CREATE TABLE indexing ("
@@ -182,15 +195,6 @@ public void should_flag_map_index_type_as_entries() {
182195
+ "otherwise, it would require deploying an actual custom index class into the C* test cluster")
183196
public void should_parse_custom_index_options() {
184197
TableMetadata table = getTable("indexing");
185-
ColumnDefinitions defs = new ColumnDefinitions(new ColumnDefinitions.Definition[] {
186-
definition(COLUMN_NAME, text()),
187-
definition(COMPONENT_INDEX, cint()),
188-
definition(KIND, text()),
189-
definition(INDEX_NAME, text()),
190-
definition(INDEX_TYPE, text()),
191-
definition(VALIDATOR, text()),
192-
definition(INDEX_OPTIONS, text())
193-
});
194198
List<ByteBuffer> data = ImmutableList.of(
195199
wrap("text_column"), // column name
196200
wrap(0), // component index
@@ -213,15 +217,50 @@ public void should_parse_custom_index_options() {
213217
+ "USING 'dummy.DummyIndex' WITH OPTIONS = {'foo' : 'bar', 'class_name' : 'dummy.DummyIndex'};", keyspace));
214218
}
215219

216-
private ColumnDefinitions.Definition definition(String name, DataType type) {
220+
/**
221+
* Validates a special case where a 'KEYS' index was created using thrift. In this particular case the index lacks
222+
* index_options, however the index_options value is a 'null' string rather then a null value.
223+
*
224+
* @test_category metadata
225+
* @expected_result Index properly parsed and is present.
226+
* @jira_ticket JAVA-834
227+
* @since 2.0.11, 2.1.7
228+
*/
229+
@Test(groups = "short")
230+
public void should_parse_with_null_string_index_options() {
231+
TableMetadata table = getTable("indexing");
232+
233+
List<ByteBuffer> data = ImmutableList.of(
234+
wrap("b@706172656e745f70617468"), // column name 'parent_path'
235+
ByteBuffer.allocate(0), // component index (null)
236+
wrap("regular"), // kind
237+
wrap("cfs_archive_parent_path"), // index name
238+
wrap("KEYS"), // index type
239+
wrap("org.apache.cassandra.db.marshal.BytesType"), // validator
240+
wrap("null") // index options
241+
);
242+
Row row = ArrayBackedRow.fromData(defs, M3PToken.FACTORY, ProtocolVersion.V3, data);
243+
ColumnMetadata column = ColumnMetadata.fromRaw(table, Raw.fromRow(row, VersionNumber.parse("2.1")));
244+
IndexMetadata index = column.getIndex();
245+
246+
assertThat(index).hasName("cfs_archive_parent_path")
247+
.isNotKeys() // While the index type is KEYS, since it lacks index_options it does not considered.
248+
.isNotFull()
249+
.isNotEntries()
250+
.isNotCustomIndex()
251+
.asCqlQuery(String.format("CREATE INDEX cfs_archive_parent_path ON %s.indexing (\"b@706172656e745f70617468\");", keyspace));
252+
}
253+
254+
255+
private static ColumnDefinitions.Definition definition(String name, DataType type) {
217256
return new ColumnDefinitions.Definition("ks", "table", name, type);
218257
}
219258

220-
private ByteBuffer wrap(String value) {
259+
private static ByteBuffer wrap(String value) {
221260
return ByteBuffer.wrap(value.getBytes());
222261
}
223262

224-
private ByteBuffer wrap(int number) {
263+
private static ByteBuffer wrap(int number) {
225264
return ByteBuffer.wrap(Ints.toByteArray(number));
226265
}
227266

‎driver-mapping/src/main/java/com/datastax/driver/mapping/MethodMapper.java‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
import java.lang.reflect.ParameterizedType;
2020
import java.lang.reflect.Type;
2121
import java.nio.ByteBuffer;
22+
import java.util.Set;
2223

24+
import com.google.common.collect.Sets;
2325
import com.google.common.util.concurrent.Futures;
2426
import com.google.common.util.concurrent.ListenableFuture;
2527

@@ -58,13 +60,7 @@ public void prepare(MappingManager manager, PreparedStatement ps) {
5860
this.session = manager.getSession();
5961
this.statement = ps;
6062

61-
if (method.isVarArgs())
62-
throw new IllegalArgumentException(String.format("Invalid varargs method %s in @Accessor interface"));
63-
if (ps.getVariables().size() != method.getParameterTypes().length)
64-
throw new IllegalArgumentException(String.format("The number of arguments for method %s (%d) does not match the number of bind parameters in the @Query (%d)",
65-
method.getName(), method.getParameterTypes().length, ps.getVariables().size()));
66-
67-
// TODO: we should also validate the types of the parameters...
63+
validateParameters();
6864

6965
Class<?> returnType = method.getReturnType();
7066
if (Void.TYPE.isAssignableFrom(returnType) || ResultSet.class.isAssignableFrom(returnType))
@@ -92,6 +88,30 @@ public void prepare(MappingManager manager, PreparedStatement ps) {
9288
}
9389
}
9490

91+
// Checks the method parameters against the query's bind variables
92+
private void validateParameters() {
93+
if (method.isVarArgs())
94+
throw new IllegalArgumentException(String.format("Invalid varargs method %s in @Accessor interface", method.getName()));
95+
96+
ColumnDefinitions variables = statement.getVariables();
97+
Set<String> names = Sets.newHashSet();
98+
for (ColumnDefinitions.Definition variable : variables) {
99+
names.add(variable.getName());
100+
}
101+
102+
if (method.getParameterTypes().length < names.size())
103+
throw new IllegalArgumentException(String.format("Not enough arguments for method %s, "
104+
+ "found %d but it should be at least the number of unique bind parameter names in the @Query (%d)",
105+
method.getName(), method.getParameterTypes().length, names.size()));
106+
107+
if (method.getParameterTypes().length > variables.size())
108+
throw new IllegalArgumentException(String.format("Too many arguments for method %s, "
109+
+ "found %d but it should be at most the number of bind parameters in the @Query (%d)",
110+
method.getName(), method.getParameterTypes().length, variables.size()));
111+
112+
// TODO could go further, e.g. check that the types match, inspect @Param annotations to check that all names are bound...
113+
}
114+
95115
@SuppressWarnings("rawtypes")
96116
private void mapType(MappingManager manager, Class<?> fullReturnType, Type type) {
97117

‎driver-mapping/src/test/java/com/datastax/driver/mapping/MapperAccessorParamsTest.java‎

Lines changed: 93 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,27 +17,30 @@
1717

1818
import java.util.Collection;
1919

20-
import com.datastax.driver.core.ResultSet;
2120
import com.google.common.collect.Lists;
2221
import org.testng.annotations.Test;
2322

2423
import static org.assertj.core.api.Assertions.assertThat;
2524

2625
import com.datastax.driver.core.CCMBridge;
26+
import com.datastax.driver.core.ResultSet;
27+
import com.datastax.driver.core.Row;
28+
import com.datastax.driver.core.utils.CassandraVersion;
2729
import com.datastax.driver.mapping.annotations.*;
2830

2931
public class MapperAccessorParamsTest extends CCMBridge.PerClassSingleNodeCluster {
3032
@Override
3133
protected Collection<String> getTableDefinitions() {
3234
return Lists.newArrayList(
33-
"CREATE TABLE user ( key int primary key, gender int)",
34-
"CREATE INDEX on user(gender)",
35-
"CREATE TABLE user_str ( key int primary key, gender text)",
36-
"CREATE INDEX on user_str(gender)");
35+
"CREATE TABLE user ( key int primary key, gender int, home_phone text, work_phone text)",
36+
"CREATE INDEX on user(gender)",
37+
"CREATE TABLE user_str ( key int primary key, gender text)",
38+
"CREATE INDEX on user_str(gender)"
39+
);
3740
}
3841

3942
@Test(groups = "short")
40-
void should_include_enumtype_in_accessor_ordinal() {
43+
public void should_allow_enum_as_int_in_accessor_params() {
4144
UserAccessor accessor = new MappingManager(session)
4245
.createAccessor(UserAccessor.class);
4346

@@ -52,7 +55,7 @@ void should_include_enumtype_in_accessor_ordinal() {
5255
}
5356

5457
@Test(groups = "short")
55-
void should_include_enumtype_in_accessor_string() {
58+
public void should_allow_enum_as_string_in_accessor_params() {
5659
UserAccessor accessor = new MappingManager(session)
5760
.createAccessor(UserAccessor.class);
5861

@@ -66,6 +69,51 @@ void should_include_enumtype_in_accessor_string() {
6669
assertThat(accessor.getUserStr(Enum.FEMALE).one().getKey()).isEqualTo(0);
6770
}
6871

72+
@Test(groups = "short")
73+
@CassandraVersion(major = 2.0, description = "Uses named parameters")
74+
public void should_allow_less_parameters_than_bind_markers_if_there_are_repeated_names() {
75+
UserPhoneAccessor accessor = new MappingManager(session)
76+
.createAccessor(UserPhoneAccessor.class);
77+
78+
session.execute("delete from user where key = 0");
79+
accessor.updatePhones_positional("1111", "2222", 0);
80+
assertPhonesEqual(0, "1111", "2222");
81+
82+
session.execute("delete from user where key = 0");
83+
accessor.updatePhones_named(0, "1111", "2222");
84+
assertPhonesEqual(0, "1111", "2222");
85+
86+
session.execute("delete from user where key = 0");
87+
accessor.updatePhones_fallback("1111", "2222", 0);
88+
assertPhonesEqual(0, "1111", "2222");
89+
90+
session.execute("delete from user where key = 0");
91+
accessor.updateBothPhones(0, "1111");
92+
assertPhonesEqual(0, "1111", "1111");
93+
94+
session.execute("delete from user where key = 0");
95+
accessor.updatePhones_fallback2("1111", "2222", 0);
96+
assertPhonesEqual(0, "1111", "2222");
97+
}
98+
99+
@Test(groups = "short", expectedExceptions = RuntimeException.class)
100+
public void should_fail_if_not_enough_parameters() {
101+
new MappingManager(session)
102+
.createAccessor(UserPhoneAccessor_NotEnoughParams.class);
103+
}
104+
105+
@Test(groups = "short", expectedExceptions = RuntimeException.class)
106+
public void should_fail_if_too_many_parameters() {
107+
new MappingManager(session)
108+
.createAccessor(UserPhoneAccessor_TooManyParams.class);
109+
}
110+
111+
private void assertPhonesEqual(int key, String home, String work) {
112+
Row row = session.execute("select * from user where key = " + key).one();
113+
assertThat(row.getString("home_phone")).isEqualTo(home);
114+
assertThat(row.getString("work_phone")).isEqualTo(work);
115+
}
116+
69117
enum Enum {
70118
MALE, FEMALE
71119
}
@@ -85,6 +133,44 @@ public interface UserAccessor {
85133
ResultSet addUserStr(int key, @Enumerated(EnumType.STRING) Enum value);
86134
}
87135

136+
/** Tests various ways to match method parameters to query bind markers. */
137+
@Accessor
138+
public interface UserPhoneAccessor {
139+
/** Standard positional markers */
140+
@Query("update user set home_phone = ?, work_phone = ? where key = ?")
141+
void updatePhones_positional(String homePhone, String workPhone, int key);
142+
143+
/** Standard named markers */
144+
@Query("update user set home_phone = :home, work_phone = :work where key = :key")
145+
void updatePhones_named(@Param("key") int key, @Param("home") String homePhone, @Param("work") String workPhone);
146+
147+
/** Named markers with no @Param. Should fallback to positional matching */
148+
@Query("update user set home_phone = :home, work_phone = :work where key = :key")
149+
void updatePhones_fallback(String homePhone, String workPhone, int key);
150+
151+
/** Named markers with repeated names */
152+
@Query("update user set home_phone = :phone, work_phone = :phone where key = :key")
153+
void updateBothPhones(@Param("key") int key, @Param("phone") String uniquePhone);
154+
155+
/** Named markers with repeated names, but no @Param. Should fallback to positional matching */
156+
@Query("update user set home_phone = :phone, work_phone = :phone where key = :key")
157+
void updatePhones_fallback2(String homePhone, String workPhone, int key);
158+
}
159+
160+
@Accessor
161+
public interface UserPhoneAccessor_NotEnoughParams {
162+
@SuppressWarnings("unused")
163+
@Query("update user set home_phone = :phone, work_phone = :phone where key = :key")
164+
void updateBothPhones(@Param("key") int key);
165+
}
166+
167+
@Accessor
168+
public interface UserPhoneAccessor_TooManyParams {
169+
@SuppressWarnings("unused")
170+
@Query("update user set home_phone = ?, work_phone = ? where key = ?")
171+
void updatePhones(String homePhone, String workPhone, int key, int extra);
172+
}
173+
88174
@Table(name = "user")
89175
public static class User {
90176
@PartitionKey

0 commit comments

Comments
 (0)