ContextConfiguration class is package-private in SimpleElasticsearchPersistentEntity

[mlyczek]

In our project we are extending SimpleElasticsearchPersistentEntity to slightly modify the result of getIndexCoordinates() method to add our custom logic for automatically assigning prefix to index name. That's the only modification, we want to use the rest of SimpleElasticsearchPersistentEntity as it is.

But in #1788 a new inner class ContextConfiguration was added to SimpleElasticsearchPersistentEntity with package-private access, which was also added as a parameter to constructor. This means that SimpleElasticsearchPersistentEntity can't be extended outside of org.springframework.data.elasticsearch.core.mapping package despite the fact that SimpleElasticsearchPersistentEntity itself is public. That difference in access modifiers for class and its constructor parameter is a little bit misleading (and also broke our sub-class ;) )

I would like to ask if ContextConfiguration could be made also public? Looking into the code it holds only FieldNamingStrategy which is public and a boolean whether to save type hints into documents. Those are things that are set on mapping context and could be customised there anyway, so I think that it could be made public to allow extending and instantiation of SimpleElasticsearchPersistentEntity possible again outside of its package. Or were there some important reasons for ContextConfiguration being package-private?

[sothawo]

Basically I consider SimpleElasticsearchPersistentEntity to be an implementation detail of Spring Data Elasticsearch which should be not used directly by an user of the library. But because of the package structure this need to be public.

The problem with having this many public classes in a library is, that when users miss some functionality, they tend to derive from and extend these classes, which in the long run makes it for the maintainers of a library harder to refactor, improve and add functionality to these classes without breaking things for people who derived from these classes.

I'd prefer if there is a problem or a missing functionality to have an issue opened where this problem can be discussed, and instead of having a local workaround it would be preferable to add missing functionality by contributing to Spring Data Elasticsearch .

[sothawo]

fixed and backported to 4.3.x branch

[mlyczek]

Thank you @sothawo for the fix.

I totally agree with you that encapsulation is better for libraries and gives maintainers more freedom with future refactorings. Also each public class - as you wrote - could be used by someone (like we used SimpleElasticsearchPersistentEntity in our application) and then every change could brake such usage. It would be huge effort for the team maintaining Spring Data Elasticsearch to think about each and every such class while doing changes and adding new features. And I agree too, that our approach with deriving from SimpleElasticsearchPersistentEntity is not ideal. We will rethink it to make future upgrades of Spring Data Elasticsearch easier for us. Your fix will help us at the moment. As you suggested, I will open a new issue about the prefixing functionality to discuss with you what do you think about such feature in Spring Data Elasticsearch.