Improve URL creation in the CDK

[sergei-solonitcyn]

Signed-off-by: Sergei Solonitcyn sergei.solonitcyn@zazmic.com

What

Current realization of the CDK uses straight strings concatenation to create a full url from url_base and /path.
It works for the most of cases. But the reason why it is a common-known bad practice, is a lack of flexibility.

One case, which was the initial reson of this update, is a Mailgun source, where we get a full url for next and previous pages. Using current scheme we're forced to create ugly code trying to extract parts of the url and concatenate it with existing base url. Besides all, it's not guaranteed to get right url this way, as the source doesn't define it.

So, we should use best practice to create urls in a flexible and predictable way.

How

Just replaced concatenation with urllib.parse.urljoin function.
Some of existing connectors used to rely on the old way of url creation. They also were updated according to the new one.

Recommended reading order

1. airbyte-cdk/python/airbyte_cdk/sources/streams/http/http.py
2. the rest

User Impact

It isn't expected to have any user impact.
Only one dangerous is that some of existing conenctors could be left unattended during this change.
Automatic integration tests have to show if this is the case.

[CLAassistant]

All committers have signed the CLA.

[sherifnada]

this seems like a better approach, but does it solve the point described in the PR description? also what's the need for rstrip especially if we are adding it again afterwards with + "/"?

[sherifnada]

what is the purpose of the rstrip portion?

[sergei-solonitcyn]

The idea is that we're not sure if the value has trailing slash. This way allows us to be sure.

[sergei-solonitcyn]

this seems like a better approach, but does it solve the point described in the PR description?

Yes, it allows to return full url, like https://domain.org/path/path, in our path() method, and it will replace the whole url, because urljoin works this way.

[sherifnada]

thanks for the explanation, lgtm. Can you update the CDK docs as appropriate to indicate this?

[sergei-solonitcyn]

thanks for the explanation, lgtm. Can you update the CDK docs as appropriate to indicate this?

Didn't find a place in the current version of the docs where according updates would be strictly required.
However, there is a big update of the docs ahead, where it could be mentioned.

[sherifnada]

@sergei-solonitcyn you need to publish the CDK and update version, team can help with procedure