Add Primary Key to CDK.

[davinchia]

What

Allow CDK users to set primary keys in the Streams.

Closes: #2766.

How

• Make primary key an abstract internal method, _get_source_defined_primary_keys, in the base stream class. Modify the as_airbyte_stream function to use the abstract method when setting source_defined_primary_keys.
• Create a source_defined_primary_keys property on HttpStream that is a list of strings. Implement the abstract primary keys method in HttpStream converting the list of keys to a list of list of keys. I felt reducing the interface to a list of strings allows users to avoid the confusion of a list of list of strings and only thinking about passing in all the response' fields required for the primary key.

Recommended reading order

1. core.py
2. http.py

[sherifnada]

bunch of suggestions and one question

[sherifnada]

1. _ indicates that a method is private, so it's a little idiosyncratic to call this _get....
2. Also, can we remove the source_defined? this is a source, so source_defined is redundant
3. This should be a property because in many cases this is hardcoded, so having to create a whole method for it is verbose
4. Should this return None by default instead of being an abstractmethod?

Also, right now a source has to define a list of list of strings. This supports four mutex cases:

1. single flat primary key e.g [["id"]]
2. single nested primary key e.g: `[["id_parent", "id_field"]]
3. multiple flat primary keys [["id1"], ["id2"]]
4. multiple nested primary keys [["id_parent1", "id_field1"], ["id_parent2", "id_field2"]]

My guess is that no 1 is by far the most common. So if I'm someone creating a source and I have to implement this method, I need to understand why this is a list of lists. Most people will think of the primary key as just a string, or maybe a list of strings in the composite case.

So why don't we have the following:

@property
@abstractmethod
def primary_key() -> Union[str, List[str], List[List[str]]]:
   """
   Return string if single primary key, list of strings if composite primary key, list of list of strings if composite primary key consisting of nested fields
   """ 

def _wrapped_primary_key() -> List[List[str]]:
   if isinstance(self.primary_key(), str): 
       return [[self.primary_key()]]
   elif isinstance(self.primary_key(), list):
       wrapped_key = []
       for component in self.primary_key():
           if isinstance(component, str):
             wrapped_key.append([component])
           elif isinstance(component, list):
              wrapped_key.append(component)
           else: 
              # error
    else: 
         # error must be a list or str

this way the most common case can just stick to using string like they're used to and if someone wants to go deeper there's nothing stopping them.

This is definitely a little more involved than usual but it will make for easier DX and readability in the majority case. WDYT?

If we do it this way we also won't need the helper method in http.py for wrapping the PK

[davinchia]

Spoke offline.

Agreed this approach provides sane defaults while exposing all underlying functionality.

Also agreed to remove all the source-defined bits of the variable names.

[davinchia]

Made the changes as we discussed. How does this look @sherifnada ?

[sherifnada]

small comments and one breaking change that will need to be removed but LGTM

[sherifnada]

can either keep this as source_defined_cursor? this one makes sense because the user actually can input a cursor, and this just toggles that

[sherifnada]

why set this to an empty string? I think either we make the base property not abstract and return None, or require users to choose a primary key. I'm kind of on the side of making it abstract and not overriding it here because almost all data has a primary key, so simply asking the user about the primary key will result in a much higher rate of connectors declaring primary keys.

[davinchia]

good point.

[davinchia]

@sherifnada holding off on publishing this base python version and updating all the down stream consumers since we are refactoring this. will do so in a separate PR if we need to do so before the refactor goes out.