Hi again. I had all tests pass from intellij, but i had to tweak them to also pass through mvn directly.
Also, i had an issue discovered on another test resulting in inherited properties overriding child properties.
All of those fixed.

Additionally i wanted to supply an example of usage of the new MapperConfiguration interface. It's all documented in the classes JavaDoc but just to give a quick jump start - the main idea is this:

// Let's create a simple mapping manager - that a mapping manager who inherits
// the default configuration to all mappers created through it.
MappingManager managerWithDefaultConfiguration = new MappingManager(session());
// Now let's create a custom configuration
MapperConfiguration customConfiguration = new MapperConfiguration();
// Now let's change some configuration inside, in this case, namely, we add
// "anotherProperty" to transient property names, and then we set it to
// inherit properties only from annotated classes
customConfiguration.getPropertyScanConfiguration().setExcludedProperties(ImmutableSet.of("class", "metaClass", "anotherProperty"));
customConfiguration.getPropertyScanConfiguration().getHierarchyScanStrategy().setScanOnlyAnnotatedClasses(true);
// Now we create a custom mapping manager - just for the sake of example,
// this manager will inherit given configuration to all mappers created through it.
MappingManager managerWithCustomConfiguration = new MappingManager(session(), customConfiguration);
// Now we create a mapper with all configuration set to default
Mapper<User> mapperWithDefaultConfiguration = managerWithDefaultConfiguration.mapper(User.class);
// Now we create a mapper with configuration equals to customConfiguration
// variable above
Mapper<User> mapperWithCustomConfiguration = managerWithCustomConfiguration.mapper(User.class);
// Now let's define some new custom configuration, namely, let's tell this
// configuration to ignore ALL fields, unless annotated by either of:
// PartitionKey, ClusteringColumn, Column, com.datastax.driver.mapping.annotations.Field or Computed
MapperConfiguration customConfiguration2 = new MapperConfiguration();
customConfiguration2.getPropertyScanConfiguration().setPropertyMappingStrategy(MapperConfiguration.PropertyMappingStrategy.WHITE_LIST);
// Now either or those mappers will be created with given (same) configuration
Mapper<User> mapperWithCustomConfiguration2 = managerWithDefaultConfiguration.mapper(User.class, customConfiguration2);
Mapper<User> anotherMapperWithCustomConfiguration2 = managerWithCustomConfiguration.mapper(User.class, customConfiguration2);

So I've taken some time to take a look at this PR. This is very extended work, thank you @avivcarmis for your involvement.

As an overall remark: I believe that while this is very extensive and detailed, there are a lot of new configuration classes introduced, and while reviewing I found it pretty hard to follow. Remembering all of these classes' purposes and relationships and names may be extra complications for users. (PropertyScanStrategy/PropertyScanScope/HierarchyScanStrategy/PropertyMappingStrategy/...)

So I agree with the need of a MapperConfiguration object to do this, but trying to strip down to the simplest approach that would allow to configure all that's required, without introducing too much independent classes and names (also, there's a lot of questions where my mind might not be clear on, so don't hesitate to correct me if you see there's an aspect of the problem I overpass):

• MapperConfiguration: as this is a "configuration for the mapping of the classes" couldn't it be called MappingConfiguration?

• PropertyScanConfiguration: This class seems to be only containing the other actual configurations, is this necessary? Couldn't we move the configurations up to the MapperConfiguration class, and remove this one?

• PropertyScanScope: This seems to allow both "scan fields" and "scan getters" to be true, is it actually necessary? If one sets true to both, it seems like "scan getters" takes precedence, right? Why would it take precedence? Couldn't it be a simple Enum SCAN_FIELDS or SCAN_GETTERS and people choose the one they want for a particular class?

• HierarchyScanStrategy: Is it necessary to have a specific class for that? It seems like this provides the feature of saying whether a user wants the ancestors of the class to be scanned and until where. Couldn't we just do so that people just provide a deepestAncestorClass, in the MapperConfiguration, and when it is set, the mapper will behave accordingly (will go look for ancestors until deepest achieved). This avoids the 2 extra fields (are they just the opposite of each other or is there more to the story?) boolean hierarchyScanStrategyEnabled and boolean scanOnlyAnnotatedClass and the HierarchyScanStrategy class.

Overall I agree that having all the separate fine-grained config classes kind of gives a more extensible approach, but it's quite hard to understand the meaning of all the classes. If I resume the configurations we want to expose, there are:

1. Enable Scan for ancestors, until a configurable deepest ancestor.
2. Access direct fields, Or use getters.
3. White List or Black List of fields.

Then I think simpler options directly on MapperConfiguration could be provided for these.

For 1) and 3), are these settings that make sense on the global MappingManager level? To my point of view, no, if you have 2 completely different classes to map, these settings are likely to be different. So users would more frequently want to specify these settings at the Mappers' level, then do we need to have the MapperConfiguration at the MappingManager level? I think it may not be completely necessary and can only be on the Mapper.

Thoughts?

@newkek Hi Kevin.

Okay so there's a lot to cover. For starters i'll say that i agree with all your inputs. I do have some thoughts that may make you change your mind regarding some of the comments. Let's decide which direction to go with each one.

As a general overview, i gotta say i kind of took it one step forward and instead of simply creating an interface for defining field mapping config, i tried to lay the ground for defining config for the entire mapper behavior, so that in the future we would be able to add more parameters on top of that config object instead of passing multiple config objects. Since i'm not that creative with examples, i can give you a real one, in the next PR i've pushed i've added an interface for defining naming conversion strategy (auto mapping camelCase to snake_case) - so then instead of passing (or not) one object for setting the mapping config and then passing (or not) an object for defining naming strategy, you can just pass the one global mapper config object, with just the settings you want to tweak changed, the rest left with their default values.
So this might explain the first two questions - why i've named it MapperConfiguration (it might in the future define more, hopefully all the mapper config) instead of MappingConfiguration, and why propertyScanConfiguration is a nested field within it and not at the top level.
Then again, coming to think of it, all of the mapper configuration will include mapping settings so MappingConfiguration sounds as intuitive to me. Regarding the second one (PropertyScanConfiguration), i think you have a fresher view over this and you should now know what i was trying to do, so i think you have a better position in deciding which one is more convenient (separation vs. over-complication).

Third point (PropertyScanScope) -

If one sets true to both, it seems like "scan getters" takes precedence

This is not entirely true, you may have (not that i can think of a good reason for that) some fields with getters and then some others without. You may want to give the opportunity to scan both. For example you may want to map both boo and goo:

public class Foo {
    
    @Column
    String boo;

    @Column 
    public boolean isGoo() {
        return something + someOtherThing > 1;
    }
    
}

Again, this is not a classic case so we can skip it if it's over complicating things. Also there's the last option of no fields and no getters allowed meaning no columns are mapped at all which is obviously redundant.
I think we have 3 options:

1. leave it like that
2. define enum {ONLY_GETTERS, ONLY_FIELDS, FIELDS_AND_GETTERS}
3. define enum {ONLY_GETTERS, ONLY_FIELDS}
   Again, trade off between separation vs. over-complication, whichever one you think is the most convenient.

Regarding the HierarchyScanStrategy - YES there's a bit more to the story (:
boolean hierarchyScanStrategyEnabled indeed configures whether to scan upwards,
boolean scanOnlyAnnotatedClass defines whether to scan only ancestors annotated with table classes (Table, UDT, Accessor) or allow scanning of all any class.
For example, i like to have a parent class containing dateCreated and dateModified fields and extend it for each entity. But i'm not sure whether i can annotate it with Table, since it's not really a table, i just want to inherit those fields and their behavior...
Again, it might not be necessary if we force all inherited stuff to come from annotated classes. But we don't have to, and there comes the trade-off again. When you give someone too many options - he might get lost. But then again, not having an option you would like also sucks. You think it's necessary?
Regarding the deepestAncestorClass we can set it by default to Object.class (actually it already is) making scanning enabled all the way by default, and when a user wants to disable it he performs hierarchyScanStrategy.setDeepestAllowedAncestor(null), although i think hierarchyScanStrategy.setHierarchyScanEnabled(false) is simpler. Maybe we can replace it with hierarchyScanStrategy.enable() and hierarchyScanStrategy.disable().
So we also have 3 options:

1. leave it like that
2. remove boolean hierarchyScanEnabled and use deepestAllowedAncestor == null to check enabled/disabled
3. leave the fields like they are but replace the setHierarchyScanEnabled setter with two setters enable() and disable()

Regarding the last point, i think that this settings (hierarchy scanning and whitelist vs blacklist) may absolutely be shared between classes. I actually wanted it for a project i worked on myself. I use annotate every field i want to persist with Column - for me it's easier to read the code this way - i do it across all my entity classes, and since the default strategy is black list i would have to explicitly create a config object for every mapper i create. Instead i want to pass it once and then have it for all the mappers.

So again i want to say that i do not disagree with any of the comments i just want you to know what i meant to achieve and maybe with popper documentation it may be convenient, but i think you're in a better position to decide on every one of those.
There's also little work to be done on every one of them so this is also not an issue.
Also if something was unclear let me know. I tried not to write too much although i kind of blew that either. (:

Cheers!

So this might explain the first two questions - why i've named it MapperConfiguration (it might in the future define more, hopefully all the mapper config) instead of MappingConfiguration, and why propertyScanConfiguration is a nested field within it and not at the top level.

Well, looking at the other PR (the naming strategy) isn't it a way to define "How property are going to be mapped when scanning them"? Why isn't it part of the PropertyScanConfiguration? Meaning, all the mapper does, is scanning objects' properties and mapping them, then PropertyScanConfiguration still seems like the only thing that could ever be configured on it. This class still seems superfluous to me.

About scanning for (getters && fields), I see your point. Although yeah I am not sure either if there is a need but I understand it would be a possible feature. Still, the extra class seems superfluous and I'd rather opt for a Enum config. So I'd personally leave a "?" on whether to choose option 2 or 3 for now.

For HierarchyScanStrategy, OK I see the distinction now for scanning only annotated classes VS scanning all classes. Although I'm not sure that allowing scanning only annotated classes would be such a powerful feature, and may imply that we support bad practices to the Cassandra data modelling front, but I'm not sure that's something I'd be the more appropriate to judge so ¯_(ツ)_/¯. Still, the other setting of Enabling the upward scan, and specifying a Up limit class to scan, both together seem redundant and I'd be more in favor of your option #2. And also to remove the HierarchyScanStrategy class.

After thinking more about it yeah I think the White and black listing config could be also a MappingManager level setting (thinking about JAVA-1310 so yes let's keep the MapperConfiguration on the MappingManager as well.

In general I've had my fair share of struggles when dealing with libraries that have too much configuration classes (taking the example of Astyanax), I believe the driver has a lot already, and creating a single class for just One option each time here seems too much. The more classes there are, the more potential opportunities of breaking changes, and for these reasons as you may have guessed I'm a big supporter of the KISS principle when possible. Maybe we can have the other driver devs weigh in on these points and see their points of view.

So to sum up what we have so far:

• PropertyScanConfiguration class should be removed and it's properties should be transferred to MapperConfiguration.
• PropertyScanScope class will be changed to an enum containing {ONLY_GETTERS, ONLY_FIELDS, FIELDS_AND_GETTERS}.
• HierarchyScanStrategy class will be migrated to a single field of type Class<?> indicating the deepest ancestor to scan. It's default value will be Object.class and we simply allow scanning of all classes (annotated and non-annotated), meaning that by default we scan properties up the hierarchy up until Object.class (I believe this keeps backwards compatibility as it is the current default behavior). To disable scanning all together, one has to call settings.setDeepestAncestorClass(null).

Let me know how you feel about it and i'll get right on it!

Sorry I closed this by mistake, I removed my old branch and I didn't realize it was based on it. Will reopen shortly.

I would slightly prefer MappingConfiguration as well

+1

PropertyScanConfiguration class should be removed and it's properties should be transferred to MapperConfiguration.

Actually I'm not convinced. I think we should think about what future options might come, and if it makes sense to group them all together.

Yes, I'm +0 here so let's keep it the way it is now imo.

PropertyScanScope class will be changed to an enum containing {ONLY_GETTERS, ONLY_FIELDS, FIELDS_AND_GETTERS}.

+1

+1, but with some minor changes:

1. I would name it as a strategy, stg like PropertyAccessStrategy;
2. We need to account for setters too, so I would suggest the following names: GETTERS_AND_SETTERS, FIELDS, BOTH.

HierarchyScanStrategy class will be migrated to a single field of type Class<?> indicating the deepest ancestor to scan.

+1

+1

OK, i'm summing up everything we went through so far. Anyone objecting to anything comment so i know to hold that up. Personally i'm good with everything we have. The last point should be decided on since it was left to later discussion.

1. PropertyScanScope class will be changed to an enum containing {GETTERS_AND_SETTERS, FIELDS, BOTH}.
2. HierarchyScanStrategy class will be migrated to a single field of type Class<?> indicating the deepest ancestor to scan.
3. Add this change.
4. Document the fact that MappingManager getters will account for configuration object only for the first time, then they'll be ignored as discussed here.
5. Add volatile modifier and remove copy constructors and duplication of config object on creation to allow changes to be reflected within an already created mapper as discussed here.
6. Move class docs to getter methods.
7. Move nested classes to top level ones.
8. Rename transient config properties and document transient hierarchy (config transient props vs. class annotation transient properties vs. transient annotation) as discussed here.
9. Rename PropertyMappingStrategy values to {OPT_IN, OPT_OUT} as discussed here.
10. Reuse AnnotationParser.VALID_COLUMN_ANNOTATIONS as discussed here.
11. Account for java transient modifier as discussed here (i'm actually +1 on that, fell for it myself when i started using the mapper).
12. Rename MapperConfiguration to MappingConfiguration.
13. Last thing left open is whether we want to remove PropertyScanConfiguration field and move it's props to MappingConfiguration top level.

Thanks for summing it up @avivcarmis !

I am 👍 for everything but the following:

Item 4) I am still a bit uncomfortable with it. If only the first configuration object is taken into account, there is a code smell here: it means that the configuration is meant to be manager-wide, not mapper-wide. If you agree with my diagnostic, how about creating a setter for MappingConfiguration on MappingManager and re-use that instance for all mappers? A constructor parameter would be ok as well.

Item 13) I would rather keep this class for the reasons Olivier stated.

@avivcarmis

5. Add volatile modifier and remove copy constructors and duplication of config object on creation to allow changes to be reflected within an already created mapper as discussed here.

I think the final consensus was rather to make all configuration objects immutable, ie make the fields final and remove setters.

@adutra

Item 4) I am still a bit uncomfortable with it. If only the first configuration object is taken into account, there is a code smell here: it means that the configuration is meant to be manager-wide, not mapper-wide.

The configuration object taken into account depends on the first call to manager.mapper. If you provide a configuration, it will be used for this mapper, otherwise the manager-wide configuration will be used.

@avivcarmis we plan to wrap up 3.2.0 development soon, do you have bandwidth to complete those pull requests? The roadmap looks pretty clear now, so we can help you with some of the remaining tasks (we generally squash before merging, so the commits will remain attributed to you no matter what).

Argh, got caught again by the fact that this PR was opened against the driver's existing java1310 branch
Let me see if I can fix this.

Nope, that doesn't work. This time I accidentally pushed your java1310 branch to our repo, and because the target was our repo's branch it's considered as merged 🤦‍♂️
I reset our branch to its previous state, but that didn't reset the PR's state.
Anyways, I'll start a new PR.

Resurrected as #814.
Sorry @avivcarmis, the branch is now in our repo so you won't be able to push directly to it. I suggest that you rename your old branch, pull our branch locally, and push any changes to your fork. I'll cherry pick your commits.

If you provide a configuration, it will be used for this mapper, otherwise the manager-wide configuration will be used.

Is this really useful? Can't we assume that one configuration is enough for the whole MappingManager?