Namespace should be determined by the Template's configuration (fixes multiple namespaces scenario)

[roimenashe]

Override the namespace, the aerospike template's namespace should be used in case its different from the persistent entity's namespace (default).

It fixes multiple namespaces configuration scenarios.

[Aloren]

I see the following issues with the code.

1. User will not be able to use repositories anymore if he/she wants to save data into different namespaces.
2. It looks like this is not the best place for this code to live in (since this is what converter is currently responsible for).

I think that the better approach would be to provide an additional step into the conversion logic:
https://github.com/aerospike-community/spring-data-aerospike/blob/master/src/main/java/org/springframework/data/aerospike/convert/MappingAerospikeWriteConverter.java#L77-L82

So that instead of:
data.setKey(new Key(entity.getNamespace(), entity.getSetName(), id));
we could have something like:

String namespace = namespaceProvider.ifPresent(p -> p.getNamespace(entity)).orElseGet(entity.getNamespace());

data.setKey(new Key(namespace, entity.getSetName(), id));

At a first glance it looks to me that this approach is more straightforward and also gives user possibility to still use repositories concept.

What do you think?

[roimenashe]

1. I don't see how it can happen, you cant use different namespaces with repositories ATM, each repository is configured with an AerospikeTemplate behind the scenes and the "defaultNamespace" (entity.getNamespace()) of the MappingAerospikeWriteConverter is using the write() method, the fix I added solves it by providing the namespace from the AerospikeTemplate (which is used by the repository) instead of using the entity's default, the example project I added demonstrates multiple repositories that uses different namespaces using this fix (https://github.com/roimenashe/spring-data-multiple-namespaces-new-example).

2. I agree that the converter is more suitable place but, not every call to write() method is done by the AerospikeTemplate (BaseAerospikeTemplate), so we need to provide a namespace from each flow as you mentioned - namespaceProvider,
   I think we can also use the existing AerospikeWriteData class that is passed to the write() method, it contains a Key which has a namespace field, most flows are using forWrite() method of the AerospikeWriteData class that causes the key field to be null - so replacing the key with an edited key that has namespace (in the flows that can provide a namespace).
   similar to my initial solution - but, in the converter to add something like that:

String providedNamespace = null;
Key providedKey = data.getKey();
if (providedKey != null) {
	providedNamespace = providedKey.namespace;
}
data.setKey(new Key(providedNamespace != null ? providedNamespace : entity.getNamespace(), entity.getSetName(), id));

Meaning, use the provided key only incase its provided - otherwise use the entity.getNamespace() (the default MappingAerospikeContext namespace).

@Aloren What do you think?

[Aloren]

Hmm.. You gave me an idea that looks very obvious to me now and I think everybody will be happy with it.
How about we add an optional field namespace to the @Document annotation? Currently it gives an ability to specify only set name, but I don't see any issues with supporting a namespace as well. So it will look like that:

@Document(namespace = "namespace-v1", collection = "my-set")
public class MyDocumentV1 {}

@Document(namespace = "namespace-v2", collection = "my-set")
public class MyDocumentV2 {}

[Aloren]

It will also leave all logic for choosing needed namespace tied to the persistent entity.

[roimenashe]

It sounded great for a second,
But i'm worried about couple of things:

1. What if a user wants different configuration per namespace? for example, custom conversion for a specific namespace? in your solution 1 AerospikeTemplate will be used when writing to all namespaces, maybe users wants the flexibility of using more than 1 AerospikeTemplate (different policies, different conversions etc...).
2. Namespaces are preconfigured in the aerospike.conf file - I get the feeling that it belongs at the configuration level (@Configuration) of the application, users will use this property in the @Document annotation and get a "namespace doesn't exists" exception, namespaces are not created on the flight as sets.

It is very intuitive place for the user but I think configuration of a template per namespace might be better.

[roimenashe]

1. What if a user configured both? persistent entity wins or an AerospikeTemplate wins? to which namespace the data will be written? I think we should go with the configuration one for flexibility but also clean and understandable.
   The idea of RepositoryOperationsMapping is nice and helps to map between an object and a configuration (AerospikeTemplate) but I think the current solution might be sufficient for now: define repositories, define configuration classes that links to that repositories (with @EnableAerospikeRepositories) and each configuration class extends the default data configuration beans, you can create an AerospikeTemplate of your own with the configurations/policies/converters you choose and specify it as a property (aerospikeTemplateRef) in the @EnableAerospikeRepositories annotation.
   We can definitely give it some thought - I'll consult with Ronen and Yevgeny about it.

2. Yes that can happen here too, I just wanted to point out that beans in the application configuration feels more "linked" to something defined in the aerospike.conf file than a property at the @document annotation level, thats a very minor reason of course, the first one is much more meaningful.

[Aloren]

Okay, I got your arguments. We can leave namespace handling on template side. Then shouldn’t we remove namespace handling from persistent entity completely?

[roimenashe]

Yes, I think you are right - I don't see why the persistent entity should be aware of the namespace - an AerospikeTemplate is always configured with a namespace even a default single AerospikeTemplate, we can use it instead of the entity's namespace and completely remove it from the entity. I'll work on that and re-ask for a review.
Thanks a lot Anastasiia :)

[Aloren]

Exactly, this was disturbing me. Now if we will have namespace handling in one place everything will be clear. Thanks

[roimenashe]

Pushed some changes and tested it (unit/integration tests and the example project), check out the commit comment I -added a detailed explanation there, I used something different since we are now relying that the namespace is provided from the outside (at the AerospikeTemplate level) since the converter is not exposed to the namespace directly.

[Aloren]

This method might be used by users, although it shouldn't be, so I am not sure what should be the best option here: change an existing method or add a new one?

[roimenashe]

Even if we assume the default namespace, keeping a forWrite() without a namespace can lead to logical issues - such as writing to the wrong namespace without encountering an exception.

I think we should change it and keep the new one only, since it should be used internally - if a user was using it then once he upgrades to the latest Spring Data Aerospike he should see an exception and modify its code to the correct usage approach, is it ok with you?

[Aloren]

Ok. Please state this change as a breaking change then. (We can add breaking-change label and add a new category for the release drafter for this)

[roimenashe]

Ok, sounds good.
I'll add it to the release notes and add a new category in the release drafter.

[Aloren]

Great. Thank you. (FYI: You can add labels to already merged PRs, this will update release notes on the next change)