JAVA-1310: Make mapper's ignored properties configurable by olim7t · Pull Request #814 · apache/cassandra-java-driver

olim7t · 2017-02-23T22:17:13Z

Reboot of @avivcarmis original pull request:

target 3.x branch
rebase on top of current 3.x
get rid of my initial implementation

Reminder of the remaining tasks:

PropertyScanScope class will be changed to an enum containing {GETTERS_AND_SETTERS, FIELDS, BOTH}.
HierarchyScanStrategy class will be migrated to a single field of type Class<?> indicating the deepest ancestor to scan.
=> I think it would also be useful to indicate if the bound is inclusive or not. In some cases you may have a common parent and you'll want to say "up to this parent included", but in others you may have multiple independent trees, and it will be "up to Object excluded".
~~Add this change.~~
~~Document the fact that MappingManager getters will account for configuration object only for the first time, then they'll be ignored as discussed here.~~
Make configuration immutable (maybe switch to builders) and remove duplication on mapper creation as discussed here.
Move class docs to getter methods.
Move nested classes to top level ones.
~~Rename transient config properties and document transient hierarchy (config transient props vs. class annotation transient properties vs. transient annotation) as discussed here.~~ => I've removed the annotation-level config property
~~Rename PropertyMappingStrategy values to {OPT_IN, OPT_OUT} as discussed here.~~
~~Reuse AnnotationParser.VALID_COLUMN_ANNOTATIONS as discussed here.~~
~~Account for java transient modifier as discussed here (i'm actually +1 on that, fell for it myself when i started using the mapper).~~
~~Rename MapperConfiguration to MappingConfiguration.~~
~~Last thing left open is whether we want to remove PropertyScanConfiguration field and move it's props to MappingConfiguration top level.~~ => no, let's keep it separate

olim7t · 2017-02-23T22:21:42Z

Let's finish this first and we'll focus on 1316 next.

olim7t · 2017-02-24T00:17:40Z

The code is currently broken, removing the annotation-level exclusions creates an issue with Object.getClass(): even if it's excluded in the configuration we try to create a PropertyMapper for it, which fails because there is no corresponding setter.
We should not try to create property mappers for ignored fields, I'm trying to move the checks earlier.

adutra · 2017-02-24T14:10:48Z

Ok I have completed the remaining tasks and fixed the tests.

A couple of remarks:

All classes moved to the child config package;
All classes are now immutable and use the builder pattern;
Hierarchy scan: we have previously agreed to remove/reduce HierarchyScanStrategy, but indeed we might have overlooked the inclusion/exclusion problem. I propose the following:
1. Keep HierarchyScanStrategy, replace deepestAllowedAncestor with nearestExcludedAncestor (i.e. make this the actual stop class = inclusive);
2. Remove scanOnlyAnnotatedClasses: I think I missed this parameter and it seems too much for me, and besides, we do not encourage users to inherit from annotated classes. If we want to re-introduce this parameter, then we will have to introduce a separate annotation (e.g. @MappedSuperclass).

adutra · 2017-02-24T14:26:48Z

Also added one commit to disable mapper-level configuration in favor of manager-level configuration only. I honestly do not see a valid use case for mapping the same class in two different ways under the same application, but I'm open to discussion.

olim7t · 2017-02-24T16:25:51Z

Also added one commit to disable mapper-level configuration in favor of manager-level configuration only. I honestly do not see a valid use case for mapping the same class in two different ways under the same application, but I'm open to discussion.

What about mapping two different classes with a different set of property exclusions?

avivcarmis · 2017-02-24T17:12:34Z

@olim7t cheers! sorry for not being available for a couple of days. everything looks great. let me know if there's anything else i can do regarding 1310.
At the meantime i'll try to sum everything up with 1316. Since it's based on my 1310 branch it might be a bit of a struggle to rebase it. I think before we start coding we might want to finish here, rebase and then go ahead.

adutra · 2017-02-24T18:16:45Z

What about mapping two different classes with a different set of property exclusions?

Hmm sorry but is this realistic? Even if it were, I don't think we have users willing to have this feature as of now, so why not add it later if need be? (it's not breaking)

olim7t · 2017-03-01T22:59:38Z

I added a commit for the inclusive/exclusive hierarchy bound.

Two things bother me about transient properties:

you can't remove the defaults (this could be easily addressed with an additional builder method that replaces the set instead of adding to it)
the exclusions are shared across all mapped classes and UDTs.
Per-class configuration is not a good answer to this, because UDT codecs share the configuration of the first table that required it, so there would be no deterministic way to know which one is used. And you couldn't have different exclusions for a table and its fields' UDTs either.
Another approach would be a single configuration, but with dotted notation for excluded properties. This gets very complicated (* and ** matching comes to mind), and there is currently no trivial way to pass the current prefix to a UDT codec (again because UDT codecs are shared across all mappers that need them).

I can't think of a good solution for 2. Maybe I'm overthinking this; if we're confident that shared exclusions are good enough, then we can keep the current implementation and I'm fine with a single, manager-wide configuration object.

newkek · 2017-03-02T15:15:57Z

-        public Builder withPropertyMappingStrategy(PropertyMappingStrategy propertyMappingStrategy) {
-            this.propertyMappingStrategy = propertyMappingStrategy;
+        public Builder withPropertyMappingStrategy(PropertyTransienceStrategy propertyTransienceStrategy) {
+            this.propertyTransienceStrategy = propertyTransienceStrategy;


I think withPropertyMappingStrategy needs to be renamed

Good catch thanks!

adutra · 2017-03-02T18:16:33Z

I am starting to have second thoughts about the usefulness of PropertyScanConfiguration. After my latest commits it's been reduced to nothing more than a wrapper for some strategies, so I would now be in favor of removing it altogether. Wdyt?
EDIT: besides, HierarchyScanStrategy doesn't really belong in there imo.

olim7t

(can only comment since I opened the PR)

Looks good to me with the latest changes to the strategies. PropertyAccessStrategy was the hardest to follow for me, but I think with a few changes and a bit of extra documentation it should be better.

olim7t · 2017-03-02T18:44:31Z

+
+    @Override
+    public boolean isTransient(String propertyName, Field field, Method getter, Method setter, Map<Class<? extends Annotation>, Annotation> annotations) {
+        return annotations.isEmpty() || annotations.containsKey(Transient.class);


From what I can tell, annotations is not filtered to the driver's types, so that would include a field that has any annotation except Transient.

olim7t · 2017-03-02T18:50:15Z

+     * @return a new {@link DefaultPropertyTransienceStrategy.Builder} instance.
+     */
+    public static DefaultPropertyTransienceStrategy.Builder builder() {
+        return new DefaultPropertyTransienceStrategy.Builder();


How about getting rid of the builder pattern for this class? It adds a lot of boilerplate, feels like if different behavior is needed it might take the form of alternate implementations.

olim7t · 2017-03-02T19:12:50Z

+     *
+     * @return {@code true} if the given field should be considered, {@code false} otherwise.
+     */
+    boolean isFieldAccessAllowed();


This (and isGetterSetterAccessAllowed) should be used in ReflectionUtils.scanFieldsAndProperties to completely skip scanning these members if they're not going to be used.

olim7t · 2017-03-02T19:14:29Z

+
+    /**
+     * Returns whether or not the given {@link Field} should
+     * be used when accessing its underlying property.


This threw me off at first read, generally when we say "given" it refers to a parameter. But this method has no parameters (maybe it used to?).

Sorry the docs are stale and referring to a previous version of this class.

olim7t · 2017-03-02T22:03:11Z

+import java.lang.reflect.Method;
+
+/**
+ * A strategy to determine how mapped properties are read and written.


I think we should have a basic description of the scanning/mapping process detailing where each method is invoked, because it's hard to grasp from an outsider's perspective.

olim7t · 2017-03-02T22:35:55Z

+     *                     The map will include all annotations found on the field, the getter and the setter methods.
+     * @return {@code true} if the given property is transient (i.e., non-mapped), {@code false} otherwise.
+     */
+    boolean isTransient(String propertyName, Field field, Method getter, Method setter, Map<Class<? extends Annotation>, Annotation> annotations);


I wonder if we should also include the mapped class here. It's not used right now but could prove useful. It cannot be derived from the field and getter/setter if those were inherited.

olim7t · 2017-03-02T22:38:24Z

+    /**
+     * Builder for {@link DefaultHierarchyScanStrategy} instances.
+     */
+    public static class Builder {


When I added a builder here it was mainly for consistency with other configuration classes. Since I'm now advocating for removing the builders elsewhere, this one could go too.

olim7t · 2017-03-02T22:40:29Z

+    /**
+     * Builder for {@link MappingConfiguration} instances.
+     */
+    public static class Builder {


Just for clarity, here it's justified to have a builder.

olim7t · 2017-03-02T22:42:25Z

        Map<String, Object[]> fieldsAndProperties = new HashMap<String, Object[]>();
-        Map<String, Field> fields = scanFields(baseClass);
+        List<Class<?>> classesToScan = Lists.<Class<?>>newArrayList(baseClass);
+        classesToScan.addAll(hierarchyScanStrategy.filterClassHierarchy(baseClass));


Why not require that the strategy include the base class in the list? Then we could avoid that copy.

Indeed, but wouldn't we have to check that the strategy did comply with this rule? It would make little sense to annotate a class with @Table and exclude it from mapping.

If the contract says that it must be included, and an implementation doesn't, then it's a bug in the implementation and there's little we can do about it.

olim7t · 2017-03-02T22:47:27Z

+        <method>java.lang.String[] transientProperties()</method>
+        <justification>False positive, it's an annotation and the new method has a default value</justification>
+    </difference>
+


I forgot to remove this, it's not needed anymore.

tolbertam · 2017-03-13T22:01:19Z

+ * <ol>
+ * <li>The property is annotated with {@link com.datastax.driver.mapping.annotations.Transient @Transient};</li>
+ * <li>The corresponding field is non-null and is marked with the keyword {@code transient};</li>
+ * <li>The property name has been explicitly black-listed.</li>


What does it mean for a property name to be explicitly blacklisted?

Its name was passed to the constructor. This comment needs to explain it in more detail.

Makes sense 👍

tolbertam · 2017-03-13T22:37:17Z

+            FrozenValue.class
+    );
+
+    private static final HashSet<String> DEFAULT_TRANSIENT_PROPERTIES = Sets.newHashSet(


wdyt about making this public? Although I don't expect it to be used very often, if one passes in transientProperties into the constructor they may be able to want to include these properties in addition to the ones they pass in.

tolbertam · 2017-03-13T22:44:22Z

+     */
+    public static class Builder {
+
+        private PropertyAccessStrategy propertyAccessStrategy = new DefaultPropertyAccessStrategy();


This creates new default instances every time you construct a MappingConfiguration. This may be an overoptimization, but you could have a static singleton for each strategies default implementation so we don't need to construct defaults every time. On the other hand, creating MappingConfiguration should not be very frequent, so it might not be worth it.

The problem is that I wanted to keep DefaultPropertyMapper really simple, so it's mutable, and not thread-safe. If we wanted to use a singleton pattern we would have to introduce a builder pattern as well and make the class immutable.

I think it is fine the way it is then 👍

tolbertam · 2017-03-13T22:45:07Z

+
+        /**
+         * Sets the {@link PropertyAccessStrategy property access strategy} to use.
+         * The default is {@link DefaultPropertyAccessStrategy}.


It might be nice to briefly summarize what the default strategy does for each of these, that way the user can decide if it is worth diving in to customizing their own strategy.

tolbertam · 2017-03-13T22:48:25Z


    }

-    @Test(groups = "unit", expectedExceptions = IllegalArgumentException.class,


Why are these tests no longer applicable? Shouldn't they still be invalid? Also noticed that the Invalid14 class still exists (but is no longer used)

It looks like in the case of unreadable it throws an NPE (on a Preconditions check) and in the case of unwritable it logs:

[main] WARN com.datastax.driver.mapping.config.DefaultPropertyAccessStrategy - Property 'notWritable' is not writable and will not be mapped

but doesn't throw an exception and returns an EntityMapper without any columns.

You are right, these exceptions have became log warnings, see here. Not sure though if we should throw an exception or just log a warning after all.

I think throwing an Exception is probably a better solution as the user would likely want to be made aware that they are improperly mapping their entity.

Makes sense I guess. I'll fix that.

tolbertam · 2017-03-13T22:53:00Z

+    }
+
+    @Test(groups = "short")
+    public void should_inherit_annotations_up_to_highest_ancestor_exluded() {


exluded -> excluded

tolbertam · 2017-03-13T22:54:33Z

+    @Test(groups = "short")
+    public void should_ignore_getters() {
+        MappingConfiguration conf = MappingConfiguration.builder()
+                .withPropertyAccessStrategy(new FieldOnlyPropertyAccessStrategy())


Seeing this in use, it looks a lot less complicated. User only has to pass in the strategy they are interested in, and the provided implementations seem pretty viable and should cover a majority of use cases I would hope 👍

tolbertam · 2017-03-13T22:55:36Z

+        }
+    }
+
+    public static class NoReflectionPropertyAccessStrategy implements PropertyAccessStrategy {


tolbertam · 2017-03-14T19:54:18Z

+     */
+    public static class LowerSnakeCase extends CharDelimitedNamingConvention {
+
+        public LowerSnakeCase() {


Would be nice for there to be a static member for this instead of a custom class since there's only one way to configure it.

Same for the others that only have default constructors too.

tolbertam · 2017-03-14T20:03:44Z

+
+    }
+
+    abstract public static class CharDelimitedNamingConvention implements NamingConvention {


I don't think we should make this abstract since none of its members are abstract and might be useful to create anonymous naming conventions (although not sure why anyone would do that :D).

tolbertam · 2017-03-14T20:17:50Z

+                throw new IllegalArgumentException(String.format("Property '%s': attribute 'value' of annotation @Computed is mandatory for computed properties", propertyName));
+            return expression;
+        }
+        boolean caseSensitive = false;


One thing that surprised me was that if I defined a custom NamingStrategy, unless I have a @Column annotation on my field/method with caseSensitive=true, this automatically lowercases the property name. In the case where you are using a naming strategy where case is important (like most are), you would think that case sensitivity would be implied. What do you think about changing the logic such that if there is a NamingStrategy provided by the user that caseSensitive=true is implied? Otherwise you'll have to @Column(caseSensitive=true) everything which seems less than ideal.

In addition, as you mentioned MapperCaseSensitivityTest.should_handle_case_sensitive_identifiers_when_keyspace_specified is failing. The reason is because The udt field name zipCode is being converted to zipcode because the default NamingStrategy enforces lowercasing of field name when getting the cassandra name representation.

I think it makes sense that if a user doesn't provide a NamingStrategy explicitly instead we could have a default strategy that does nothing to the input fields when mapping to cassandra column (and vice versa). We could then assume if a NamingStrategy is provided, we enforce case sensitivity, wdyt?

Column.caseSensitive should only apply to Column.name if it's explicitly provided. And an explicit column name in the annotation should override the naming strategy for this property.

I think initializing caseSensitive to true on this line would address the issue. If a naming strategy intends to ignore case, it should return lower-cased identifiers, and quoting them on line 378 has no effect.
Alternatively, we could change the flow to have two different return statements (name provided via annotation => consider caseSensitive, name provided via strategy => always quote), would make things a bit clearer I think.

Pushed a commit to implement my suggestion.

tolbertam · 2017-03-14T22:51:33Z

The API seems really nice so far, will keep reviewing and will push some more tests tomorrow.

tolbertam · 2017-03-14T22:52:05Z

can you rebase this? Would be good to do now since it's lagging a little bit behind 3.x.

olim7t · 2017-03-15T20:28:37Z

+ */
+package com.datastax.driver.mapping;
+
+public class PassThroughNamingStrategy implements NamingStrategy {


This is not used anywhere, do we need to provide it? Looks simple enough to reimplement if someone needs it.

olim7t · 2017-03-15T21:04:08Z

+                throw new IllegalArgumentException(String.format("Property '%s': attribute 'value' of annotation @Computed is mandatory for computed properties", propertyName));
+            return expression;
+        }
+        boolean caseSensitive = false;


Pushed a commit to implement my suggestion.

olim7t · 2017-03-15T21:06:32Z


        @PartitionKey
-        @Column(caseSensitive = true)
+        @Column(name = "userId", caseSensitive = true)


This is a behavioral change compared to the current version, but I think it's the reasonable thing to do. If someone uses case-sensitive Cassandra names with a strategy, they will likely have the strategy return mixed-case names rather than annotate all their fields.

I like it! Also fixes my other complaint about having to annotate everything with @Column(caseSensitive=true).

Yes this is a behavioral change but it solves the problem elegantly, so 👍

Added note about that to upgrade guide in 3b86cef (hope that looks ok, feel free to adjust)

olim7t · 2017-03-15T23:40:15Z

+/**
+ * Implementations of industry common naming conventions.
+ */
+public class CommonNamingConventions {


NamingConventions would be sufficient IMO

olim7t · 2017-03-15T23:46:32Z

- * The following parts can be configured:
- * <ul>
- * <li>{@link PropertyMapper}: the mapper component that maps Java properties to table columns and UDT fields.</li>
- * </ul>


This is a text description of the builder methods, I think the API is obvious enough that we can remove it, and avoid having to keep it in sync.

olim7t · 2017-03-15T23:47:44Z

+                CommonNamingConventions.LOWER_SNAKE_CASE));
+```
+
+There is more to `DefaultPropertyMapper`; see the Javadocs and implementation for details.


I didn't want to cover every aspect, customizing the configuration is involved anyway, people who do it will likely look at the implementation.

avivcarmis · 2017-03-16T12:36:34Z

Happy to see things kicking in...looking forward to using the new API.
Let me know if I can still help somehow.

adutra · 2017-03-16T16:38:50Z

I think we are close to code-complete here. The only minor nit I would like to offer for discussion is: the builder pattern in MapppingConfiguration. Like I explained for PropertyMapper, these configuration classes are not expected to execute concurrently and therefore do no need to be strict about immutability or thread-safety. Under this perspective, the builder pattern seems to bit a bit overkill; in addition, a simple, mutable version of MappingConfiguration with a public default constructor would be easier to extend (if that ever makes sense one day).

olim7t · 2017-03-16T16:41:09Z

rebased on 3.x

tolbertam · 2017-03-16T18:31:47Z

A few test issues popped up after rebase, looking into it, probably an easy explanation.

tolbertam · 2017-03-16T18:51:08Z

It looks like the issue has to do with fields being quoted (presumably for case sensitivity) in a UDT when they don't always need to be:

Will see where/why that could be happening.

tolbertam · 2017-03-16T18:57:17Z

+        mappedName = namingStrategy.toCassandraName(propertyName);
+        if (mappedName == null || mappedName.isEmpty())
+            throw new IllegalArgumentException(String.format("Property '%s': could not infer mapped name", propertyName));
+        return Metadata.quote(mappedName);


If the Field/Column doesn't have a name set, we depend on the field name, this explains the tests failing for UDT mapper tests(see this comment). I think we could just use handleId in MappedUDTCodec when looking up columnMappers and it should fix it, wdyt?

Actually handleId does the opposite of what i want there (strips quotes).

Maybe what we should be doing here is using Metadata.escapeId (note: this method is not accessible from this scope) instead of quote, which would only quote the name if it needs to be quoted. MappedUDTCodec would then work as is I believe. All the MapperUDTTest tests pass with that change.

It looks like doing something like that will fix all the test issues since they seem to be tied to quoting.

Yes, escapeId is the solution here. We can't always quote the name because it's compared against the schema metadata coming from Cassandra, where identifiers are only quoted if necessary.
I think we can make handleId public instead of duplicating the logic, it looks like something that might be useful for end users occasionally.

newkek

lgtm, had some comments. Mostly I was wondering if we could get rid of exposing the PropertyMapper class because I'm not sure if anybody would want to implement a different PropertyMapper considering we have the different mapping Strategies options. Imo the strategies offer great configuration already, so I'd be in favor of allowing to set the strategies via the MappingConfiguration directly, and not expose the PropertyMapper/DefaultPropertyMapper. The rest looks good, I also may have missed a part of the conversation but I was wondering if there is really a use for the javaConvention part of the NamingStrategy, do we really expect that many people to explicitly not follow the naming conventions of Java, in a Java program? There are probably use cases I am not aware of.

newkek · 2017-03-16T19:11:21Z

-    public String toString() {
-        return propertyName;
-    }
+public interface PropertyMapper {


I don't understand the need of an interface here, could there be another kind of implementation? Also I don't understand why we expose those 2 methods considering their default (and only) implementation will call the same method internally?

could there be another kind of implementation?

There used to be a test that completely bypassed reflection in favor of hard-coded casting (in case the mapped classes are known in advance), but I can't find it anymore.

Also I don't understand why we expose those 2 methods considering their default (and only) implementation will call the same method internally

They call the same method but the second argument (the annotations allowed on the properties) is different.

My intention was to distinguish two populations: "regular" and "advanced" users. Regular users would typically use DefaultPropertyMapper and just configure the strategies associated with it. Advanced ones would rather extend it or re-implement it completely.
The test that completely bypasses reflection has indeed been removed, not because it doesn't work anymore, but because it's imo something we should leave for the advanced users to do. But please trust me on this, I have tried, and it works (it's just a considerable amount of code to write).

To be more specific: to bypass reflection you need at a bare minimum to 1) override DefaultPropertyMapper.createMappedProperty() and 2) provide your own implementation of MappedProperty.

Fair enough, I am not convinced for the target of "advanced" users but we'll see.

newkek · 2017-03-16T19:31:29Z

+     * @param transientPropertyNames a set of property names to exclude from mapping; may not be {@code null}. This
+     *                               will completely replace any names already configured for this object.
+     */
+    public DefaultPropertyMapper setTransientPropertyNames(Set<String> transientPropertyNames) {


Is it possible now to specify a white list of properties to map? So that you can either: specify a list of properties to exclude, or specify a list of only the properties that will be included in the mapping (not considering the annotations).
I thought the original fix was allowing that but I just checked because I remember the WHITE_LIST enum but it was actually allowing to do the same thing than now, but what do you think of the idea?

That feels less commonly needed, and as a last resort you can override isTransient.

I think what was called WHITE_LIST at some point was actually the equivalent of PropertyTransienceStrategy.OPT_IN.

Seems it could be useful for when doing some sort of inheritance in the Java objects when having only one Cassandra table to map to. By providing a list of only the properties that are supposed to be mapped, you can map a table with a Java object, or its child, or its child's childs, etc, without having to explicitly exclude the children's properties via a black list each time. Or when you're denormalizing tables in C*, it could help selecting only some fields to map, without worrying about the other fields in the different denormalized tables, or other fields in the mapped object. Seems useful to me but that's a subjective opinion and we don't really have external feedback requesting this specifically so... ok.

newkek · 2017-03-16T19:50:31Z

+ * achieve pre-<a href="https://datastax-oss.atlassian.net/browse/JAVA-541">JAVA-541</a>
+ * behavior.
+ */
+public class MappedClassesOnlyHierarchyScanStrategy implements HierarchyScanStrategy {


It seems like the purpose of this class is the same than if the user had used new DefaultHierarchyScanStrategy(TheMappedClass.class, true), is it ok to expose those 2 ways for doing the same thing?

But TheMappedClass.class is not constant, filterClassHierarchy gets called for each class.

Ok, fair enough.

olim7t · 2017-03-16T22:54:38Z

I'm not sure if anybody would want to implement a different PropertyMapper considering we have the different mapping Strategies options

There are things that you can do by overriding protected methods that go beyond strategies (e.g. your comment about white list).

if there is really a use for the javaConvention part of the NamingStrategy

One example was if you use field access and your naming convention includes a prefix, for example private String _userName or private String mUserName (personally I don't like it but you see it sometimes).

adutra · 2017-03-16T23:21:59Z

if there is really a use for the javaConvention part of the NamingStrategy

Hungarian notation is still pretty much common, although I admit I've been seeing it less and less frequently.

tolbertam · 2017-03-17T02:05:27Z

        <justification>False positive, the enclosing class is package-private so this was never exposed</justification>
    </difference>

+    <difference>


I think this is ok? The method was protected, so in theory classes that extend BuiltStatement could access it, but you can't extend BuiltStatement directly since its abstract method buildQueryString is package private. I guess you could extend a subclass, but it isn't clear which one you could extend since they all seem to have package private constructors.

I've re-added the method and deprecated it. It doesn't hurt to keep it and it's the cleanest/safest way.

Sounds good 👍

tolbertam · 2017-03-17T14:23:52Z

+
+```java
+PropertyMapper propertyMapper = new DefaultPropertyMapper()
+        .setPropertyTransienceStrategy(PropertyTransienceStrategy.OPT_OUT);


Shouldn't this be OPT_IN instead of OPT_OUT based on the description above?

Fixed (assuming i'm right since default is OPT_OUT)

tolbertam · 2017-03-17T14:26:46Z

 [@QueryParameters]: http://docs.datastax.com/en/drivers/java/3.0/com/datastax/driver/mapping/annotations/QueryParameters.html
+
+
+### Mapping configuration


This is probably the right amount of documentation 👍 . I think if the user wants to go beyond this they probably need to take time to understand the code. We can always add more based on questions we get from community.

tolbertam · 2017-03-17T19:57:01Z

Pushed up some more tests. I think at this point we are pretty good on coverage (can always add more to test more permutations but I think what we have is adequate enough). All of my comments/concerns have been addressed and things are working pretty well. 👍 'ing the PR, but will follow convo/commits as they come.

tolbertam · 2017-03-20T16:19:46Z

I introduced a test failure (not using frozen on UDT)...fixing.

olim7t · 2017-03-22T22:12:18Z

@adutra

these configuration classes are not expected to execute concurrently and therefore do no need to be strict about immutability or thread-safety

To put it differently, attempting to change the configuration at runtime would have unpredictable consequences.
PropertyMapper is pretty safe since the configuration exposes an interface with no mutating methods. As for MappingConfiguration, I'd rather keep it immutable to make it clear that it's not supposed to change; a builder is overkill for only one field, but it's better for future backward compatibility.
So I vote to keep things as is.

@adutra

JAVA-1310: Make mapper's ignored properties configurable. JAVA-1316: Add strategy for resolving properties into CQL names. This is a squashed commit, including significant contributions from @adutra and @tolbertam.

olim7t · 2017-03-23T16:14:54Z

I used the "squash and merge" strategy and I just realized it doesn't preserve the original commit author 😞 Sorry @avivcarmis I would have mentioned you in the commit message if I had known that.

olim7t added this to the 3.2.0 milestone Feb 23, 2017

olim7t mentioned this pull request Feb 23, 2017

JAVA-1310: Implementation of MapperConfiguration and basic behavior #771

Merged

newkek reviewed Mar 2, 2017

View reviewed changes

adutra force-pushed the java1310 branch 2 times, most recently from 0d3ec06 to 5f7f8a5 Compare March 2, 2017 18:05

olim7t commented Mar 2, 2017

View reviewed changes

adutra force-pushed the java1310 branch from eb8c745 to b45490b Compare March 6, 2017 16:14

tolbertam reviewed Mar 13, 2017

View reviewed changes

tolbertam reviewed Mar 14, 2017

View reviewed changes

tolbertam mentioned this pull request Mar 14, 2017

JAVA-1316: Naming strategy support in mapper configuration level #778

Closed

olim7t commented Mar 15, 2017

View reviewed changes

olim7t force-pushed the java1310 branch from 66d2f17 to 96fbd35 Compare March 16, 2017 16:39

tolbertam reviewed Mar 16, 2017

View reviewed changes

newkek reviewed Mar 16, 2017

View reviewed changes

tolbertam reviewed Mar 17, 2017

View reviewed changes

tolbertam approved these changes Mar 17, 2017

View reviewed changes

adutra approved these changes Mar 23, 2017

View reviewed changes

olim7t force-pushed the java1310 branch 2 times, most recently from 6e232f6 to 3fd2e05 Compare March 23, 2017 16:09

JAVA-1310/JAVA-1316: Allow more customization of mapper behavior

afd237f

JAVA-1310: Make mapper's ignored properties configurable. JAVA-1316: Add strategy for resolving properties into CQL names. This is a squashed commit, including significant contributions from @adutra and @tolbertam.

olim7t force-pushed the java1310 branch from 3fd2e05 to afd237f Compare March 23, 2017 16:10

olim7t merged commit 8645499 into 3.x Mar 23, 2017

olim7t deleted the java1310 branch March 23, 2017 16:11


		}

		@Test(groups = "unit", expectedExceptions = IllegalArgumentException.class,


		}

		abstract public static class CharDelimitedNamingConvention implements NamingConvention {

		[@QueryParameters]: http://docs.datastax.com/en/drivers/java/3.0/com/datastax/driver/mapping/annotations/QueryParameters.html


		### Mapping configuration

Conversation

olim7t commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olim7t commented Feb 23, 2017

Uh oh!

olim7t commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adutra commented Feb 24, 2017

Uh oh!

adutra commented Feb 24, 2017

Uh oh!

olim7t commented Feb 24, 2017

Uh oh!

avivcarmis commented Feb 24, 2017

Uh oh!

adutra commented Feb 24, 2017

Uh oh!

olim7t commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olim7t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olim7t Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolbertam Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olim7t commented Feb 23, 2017 •

edited

Loading

olim7t commented Feb 24, 2017 •

edited

Loading

adutra commented Mar 2, 2017 •

edited

Loading

adutra Mar 3, 2017 •

edited

Loading

olim7t Mar 3, 2017 •

edited

Loading

tolbertam Mar 13, 2017 •

edited

Loading

tolbertam Mar 14, 2017 •

edited

Loading

tolbertam Mar 14, 2017 •

edited

Loading