Extract common code for ES and OS into a base class

[masci]

Related issue: #1764
Proposed changes:

• Create a new base class, ElasticsearchCommon implementing the code that Elasticsearch and Opensearch have in common at this moment
• There should be no functional change, with ElasticsearchDocumentStore implementing the same type as before

Before

┌───────────────────────────┐                     
│                           │                     
│   KeywordDocumentStore    │                     
│                           │                     
│                           │                     
└───────────────────────────┘                     
              ▲                                   
              │                                   
              │                                   
┌───────────────────────────┐                     
│                           │                     
│ElasticsearchDocumentStore │       ┌────────────┐
│                           │──────▶│ ES Client  │
│                           │       └────────────┘
└───────────────────────────┘                     
              ▲                                   
              │                                   
              │                                   
┌───────────────────────────┐                     
│                           │                     
│  OpenSearchDocumentStore  │                     
│                           │                     
│                           │                     
└───────────────────────────┘                     

After

┌───────────────────────────┐                     
│                           │                     
│   KeywordDocumentStore    │                     
│                           │                     
│                           │                     
└───────────────────────────┘                     
              ▲                                   
              │                                   
              │                                   
┌───────────────────────────┐                     
│                           │                     
│    ElasticsearchCommon    │                     
│                           │                     
│                           │                     
└───────────────────────────┘                     
              ▲                                   
              │                                   
              │                                   
┌───────────────────────────┐                     
│                           │                     
│ElasticsearchDocumentStore │       ┌────────────┐
│                           │──────▶│ ES Client  │
│                           │       └────────────┘
└───────────────────────────┘                     
              ▲                                   
              │                                   
              │                                   
┌───────────────────────────┐                     
│                           │                     
│  OpenSearchDocumentStore  │                     
│                           │                     
│                           │                     
└───────────────────────────┘                     

Next step (upcoming PR)

                          ┌───────────────────────────┐                                            
                          │                           │                                            
                          │   KeywordDocumentStore    │                                            
                          │                           │                                            
                          │                           │                                            
                          └───────────────────────────┘                                            
                                        ▲                                                          
                                        │                                                          
                                        │                                                          
                          ┌───────────────────────────┐                                            
                          │                           │                                            
                          │    ElasticsearchCommon    │                                            
                          │                           │                                            
                          │                           │                                            
                          └───────────────────────────┘                                            
                                        ▲                                                          
              ┌─────────────────────────┴────────────────────────┐                                 
              │                                                  │                                 
┌───────────────────────────┐                      ┌───────────────────────────┐                   
│                           │                      │                           │                   
│ElasticsearchDocumentStore │      ┌────────────┐  │  OpenSearchDocumentStore  │     ┌────────────┐
│                           │─────▶│ ES Client  │  │                           │────▶│ OS Client  │
│                           │      └────────────┘  │                           │     └────────────┘
└───────────────────────────┘                      └───────────────────────────┘                   

Status (please check what you already did):

• First draft (up for discussions & feedback)
• Final code
• Enable actions on your fork
• Added tests
• Updated documentation

[ZanSara]

Looks good! I have two questions regarding ElasticsearchCommon, nothing blocking though 🙂

[ZanSara]

Two small issues here:

• I think this should be an abstract class, because it's not meant to be implemented. It has no abstract methods for now, but I imagine it will once you move to the final hierarchy that you designed in the PR description. So if this is meant to be temporary we can make this abstract later.

• I'm not too fond of the name. We use to prefix these base abstract classes with Base, so if I had to follow the pattern, this would be called BaseElasticsearchDocumentStore or anyway Base[Something]DocumentStore. The current ElasticsearchCommon is way too generic imho, doesn't imply anywhere we're even talking about a docstore. If this objection sounds too far-fetched, be aware that until very short ago we had an ElasticsearchRetriever, so ElasticsearchCommon sounds like a set of functions that could serve both, which is not correct.

[masci]

• I think this should be an abstract class,

I tried, but in the end we don't need abstract methods here, and adding one just for the sake of make the class not instantiable is wrong IMO. I think good documentation and clean code is enough in Pythonland, but If this is important, I'd rather raise an error in the __new__ method of the class

I'm not too fond of the name.

Me too, it's pretty bad and this is a code smell - a class hierarchy isn't probably the right tool here, but this is way off topic 😅. I'll change it to BaseElasticsearchDocumentStore.

Anyways, you touch an interesting point here:

If this objection sounds too far-fetched, be aware that until very short ago we had an ElasticsearchRetriever, so ElasticsearchCommon sounds like a set of functions that could serve both, which is not correct.

I disagree - when thinking about names we should take into consideration the fully qualified name: ElasticsearchCommon alone is obviously bad, but its name is actually haystack.document_stores.elasticsearch. ElasticsearchCommon. It's not unusual to leverage fully qualified names in API design: for example, in Django you have a class named DatabaseWrapper in every backend instead of MysqlWrapper, PostgresqlWrapper and so on. Not something I would necessarily add to Haystack, just wanted to mention that packages and modules should be used to provide namespacing when writing Python APIs, as opposed to put all the effort in function or class names.

[ZanSara]

For the abstract class, I agree that if we have to do workarounds to make it really abstract then it's worthless. +1 for keeping it as it is.

For the name, nice idea! I still think it's better to use BaseElasticsearchDocumentStore here and I'm glad we agree, but I'll keep it in mind for other situations. Sounds like a handy solution to keep names short.