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

Adding element to set via update query fails [DATACASS-770] #906

Closed
spring-projects-issues opened this issue May 28, 2020 · 6 comments
Closed
Assignees

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented May 28, 2020

Marko opened DATACASS-770 and commented

When you try to add a new element to a Set the query fails since it is transformed as adding an element to a List.

Example:

// Entity we work with
@Table("foo")
public class Foo {
@PrimaryKeyColumn(name = "foo_id", ordinal = 0, type = PrimaryKeyType.PARTITIONED) private UUID fooId; 
@PrimaryKeyColumn(name = "type_id", ordinal = 1, type = PrimaryKeyType.CLUSTERED, ordering = Ordering.ASCENDING) private String typeId;
@Column("set") @CassandraType(type = CassandraType.Name.SET, typeArguments = CassandraType.Name.UUID) private Set<UUID> set;
}
// This code should add new element to the "set"
cassandraOperations.update(Foo.class) .matching(query(where("foo_id").is(fooId)).and(where("type_id").is(typeId))) .apply(empty().addTo("set").appendAll(newUUIDElement));
// The CQL query produced is following:
UPDATE foo SET set=set+[3fa85f64-5717-4562-b3fc-2c963f66afa6] WHERE foo_id=3fa85f64-5717-4562-b3fc-2c963f66afa6 AND type_id='7be7ba4a4dd84013a8b22248e85090b2'];
//instead of:
UPDATE foo SET set=set+{3fa85f64-5717-4562-b3fc-2c963f66afa6} WHERE foo_id=3fa85f64-5717-4562-b3fc-2c963f66afa6 AND type_id='7be7ba4a4dd84013a8b22248e85090b2'];
 

 

While debugging I found the source of a problem. In class DefaultColumnTypeResolved.java in package org.springframework.data.cassandra.core.convert in the line 293. Currently is:

 

if (value instanceof Set) { 
   return ColumnType.listOf(DefaultColumnType.OBJECT); 
}
// I assume that it should be like this
if (value instanceof Set) { 
   return ColumnType.setOf(DefaultColumnType.OBJECT); 
}

 

 

 


Affects: 3.0 GA (Neumann)

Referenced from: pull request #174

Backported to: 3.0.1 (Neumann SR1)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 28, 2020

Mark Paluch commented

Good catch. Do you want to submit a pull request to fix the issue as you've already went through the code?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 28, 2020

Marko commented

Yes, I will do it today

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 28, 2020

Mark Paluch commented

Can you share a bit more of details here as the unit tests in the PR succeed without the change and integration tests succeed as well. Fixing the bug in DefaultColumnTypeResolver is necessary but I'd like to understand the full context so we can adjust the unit tests and see whether there's another issue in this arrangement

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 28, 2020

Marko commented

Obviously I was a little bit naive since it seemed like an obvious bug :) I drilled down a bit and this are my findings:

@Table("foo") 
public class Foo { 
@PrimaryKeyColumn(name = "foo_id", ordinal = 0, type = PrimaryKeyType.PARTITIONED) 
private UUID fooId; 
@PrimaryKeyColumn(name = "type_id", ordinal = 1, type = PrimaryKeyType.CLUSTERED, ordering = Ordering.ASCENDING)
private String typeId; 
 
@Column("set_col") @CassandraType(type = CassandraType.Name.SET, typeArguments = CassandraType.Name.UUID) 
private Set<UUID> set; }

Everything works as expected (without fix) as long as you use attribute name (set) as a column name in addTo() and not column name used in database ("set_col").

addTo("set") > works (Tests with success: 2, 3, 8, 9, other (with "sel_col") fail). Test number can be seen below in Tests, next to DATACASS-770.

If we fix the line 291 in DefaultColumnTypeResolved.java (change from listOf -> setOf), also the tests 6 and 7 pass. The problem remains with the appendAll and prependAll.

I dug further and found that the problem is that the field property is not set if we have a column name ("set_col") and not attribute name ("set"). The reason is that field.property (QueryMapper.java line 123) info is obtained from the map by the attribute name ("set"), which returns null in case of a column name ("set_col"). Because of that, the object is not converted to the correct value as in case of attribute name (Updatemapper.java line 205 -> if clause is false in case of column name ("set_col") )

I am not sure if it was meant by design to even use the column names (as they are in the database), but on the other side I used them on all other parts of the query, and even the documentation says (columnName)? What would you suggest how to resolve this issue, if it even is an issue? :) Let me know what should I include in the pull request.

 

 

Tests src/test/java/org/springframework/data/cassandra/core/convert/UpdateMapperUnitTests.java:

@Test // DATACASS-770 - TEST 1   
public void shouldPrependAllToSetViaColumnName() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").prependAll("foo", currency),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = {'foo','Euro'} + set_col");    }
    
@Test // DATACASS-770 - TEST 2  
public void shouldPrependAllToSet() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set").prependAll("foo", currency),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = {'foo','Euro'} + set_col");    }
    
@Test // DATACASS-770 - TEST 3    
public void shouldAppendAllToSet() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set").appendAll("foo", currency),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'foo','Euro'}");    }
    
@Test // DATACASS-770 - TEST 4   
public void shouldAppendAllToSetViaColumnName() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").appendAll("foo", currency),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'foo','Euro'}");    }
    
@Test // DATACASS-770 - TEST 5    
public void shouldAppendAllToSetViaColumnNameSingleElement() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").appendAll(currencyUSD),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'US Dollar'}");    }
    
@Test // DATACASS-770 - TEST 6    
public void shouldAppendAllToSetViaColumnNameCollectionOfElement() {        

Set<Currency> tmp = new LinkedHashSet<>();        
tmp.add(currencyUSD);        
tmp.add(currency);        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").appendAll(tmp),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'US Dollar','Euro'}");    
}
    
@Test // DATACASS-770 - TEST 7    
public void shouldAppendToSetViaColumnName() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").append("foo"),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'foo'}");    
}
    
@Test // DATACASS-770 - TEST 8   
public void shouldAppendToSet() {
        
Update update = updateMapper.getMappedObject(Update.empty().addTo("set").append("foo"),                persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col + {'foo'}");    
}
    
@Test // DATACASS-770 - TEST 9    
public void shouldRemoveFromSet() {
        
Update update = updateMapper.getMappedObject(Update.empty().remove("set", currency), persistentEntity);
        
assertThat(update.getUpdateOperations()).hasSize(1);        
assertThat(update.toString()).isEqualTo("set_col = set_col - {'Euro'}");    
}


 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 29, 2020

Mark Paluch commented

Thanks for your feedback. While you can use both, property names and column names, we need to look at what's possible in which case.

Column names are generally not attempted for resolution against the model. Therefore we need to use in query the type information that is available from the query/update. In this case, prependAll(Object) and prependAll(object1, object2) provide only a that it's a collection-like value to be prepended and we decided to default to List.

prependAll(new HashSet(…)) is required to treat the value as Set (and that is what is broken).

The preferred approach, therefore, is to use property names as we can map query and update objects against these. Collection values from e.g. prependAll can be converted into the property target type (or the one that is annotated through @CassandraType) and we should end up with proper mapping.

Feel free to include two tests for the cases of column name usage and property name usage in UpdateMapperTests and please do not mix value types in prepend/appendAll

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 29, 2020

Marko commented

I added additional tests as suggested and committed all the changes. I removed the following test:

public void shouldAppendToSetViaColumnName() { 
Update update = updateMapper.getMappedObject(Update.empty().addTo("set_col").append("foo"), persistentEntity);
assertThat(update.getUpdateOperations()).hasSize(1); 
assertThat(update.toString()).isEqualTo("set_col = set_col + {'foo'}"); 
}

since this is only working, because the current implementation of append() calls appendAll() with a collection of a Set type.

public Update append(Object value) { 
return appendAll(Collections.singleton(value)); 
}

Thanks for all the clarifications. I also managed to get my code working now.  :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.