add overloads with ttl parameter to Insert functions on ICqlWriteClient and… #192
Conversation
|
This might be a dupe of #90, however the comments there by @jorgebay indicate that this is undesired on I will review how to do this using the linq component. Perhaps this is not necessary. |
| @@ -29,14 +29,14 @@ public CqlBatch(MapperFactory mapperFactory, CqlGenerator cqlGenerator) | |||
| _statements = new List<Cql>(); | |||
| } | |||
|
|
|||
| public void Insert<T>(T poco, CqlQueryOptions queryOptions = null) | |||
| public void Insert<T>(T poco, CqlQueryOptions queryOptions = null, int? ttl = null) | |||
There was a problem hiding this comment.
Creating a new parameter (even if its with a default value) is a binary break, we must expose it as an overload if we want to deliver it in a minor / patch version: http://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net
|
Good call for making this only available for the insert methods, as those relies on query generation. Update methods can include it within the query. Apart from the methods overload comment, looks good to me. I will create a separate ticket for timestamp support in the mapper, that do belong at |
…nt and ICqlWriteAsyncClient (fixes CSHARP-395)
|
Updated PR with new overloads. If I overload and add it as optional I get a R# warning that
Not exactly a fatal warning, but thought I'd take the hint and not overload with optionals. I'll defer to your judgement if these signatures are correct. I'm fine reworking them if you'd like. |
collinsauve
changed the title
@jorgebay I only excluded it because currently we do not have a requirement to set TTL on update. Thinking about it more closely I'm actually not sure I agree that it should be excluded :) although maybe I'll do another PR to keep this one simpler. Those that are using the mapper for the poco->Cql generation may wish to set TTL on update. This is my reasoning for wanting TTL on insert - I don't want people to have to craft their own CQL for inserts. |
Remember that users can specify the query in the mapper.Update("UPDATE tbl SET x = ? WHERE id = ? TTL ?", x, id, ttl); |
| @@ -468,6 +483,11 @@ public AppliedInfo<T> InsertIfNotExists<T>(T poco, bool insertNulls, CqlQueryOpt | |||
| return TaskHelper.WaitToComplete(InsertIfNotExistsAsync(poco, insertNulls, queryOptions), _queryAbortTimeout); | |||
| } | |||
|
|
|||
| public AppliedInfo<T> InsertIfNotExists<T>(T poco, bool insertNulls, int? ttl, CqlQueryOptions queryOptions = null) | |||
| { | |||
| throw new NotImplementedException(); | |||
|
Pushed new commits to address the issues raised by @jorgebay. |
For https://datastax-oss.atlassian.net/browse/CSHARP-395 (Mapper: Support TTL and timestamp)
When trying to implement this, I was looking at what
CqlQueryOptionsis used for. These properties are typically used to decorate anIStatement. In this case I believe the ttl is only put into the actual query, rather than serializing it into query_parameters.Also considering that:
CqlQueryOptionsis mainly write-onlyCopyOptionsToStatement(IStatement)I would need to break this quasi-write-only idiom in order to read from
MapperandCqlBatch.If you still feel ttl should be on
CqlQueryOptions, let me know and I'll file a new PR.