[Oracle] Add port to DSN

[malthe]

This adds the port (if specified) to the DSN string.

Previously, a port would be ignored.

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[potiuk]

Could you please add a unit test for that ?

[malthe]

@potiuk done – and I have fixed a minor bug (it shouldn't add the port suffix if it's the default port).

[uranusjr]

I'd probably removed the 1521 check (or change it to check for None instead); even if the port is the default, there's no harm including it in the DSN.

[malthe]

@uranusjr it seems to me that conn.schema should be added there as well:

if conn.schema is not None:
    dsn += "/" + conn.schema

E.g. localhost:1521/xe.

That seems to be right based on my experiments in passing in such a connection string.

[potiuk]

Indeed looks like at least service_name should be added as well:

https://cx-oracle.readthedocs.io/en/latest/user_guide/connection_handling.html#establishing-database-connections

[malthe]

@potiuk yes, but when you're just providing a connection string, service_name is not what gets populated, conn.schema is.

So if we want host:port/<something> to be translated back to host:port/<something>, then it seems conn.schema is the way to go.

[potiuk]

So if we want host:port/ to be translated back to host:port/, then it seems conn.schema is the way to go.

Hmm. I think this is not how (at least originally) the oracle connection was designed so far - it never mentions schema in the docs and it makes a clear distinction between SID (seems older system) and Service_name - seems like a newer one.

The docs https://airflow.apache.org/docs/apache-airflow-providers-oracle/stable/connections/oracle.html are slightly misleading I think (They do not mention that service_name is passed as service_name extra.

However I think I am perfectly fine with using schema as service_name in case the latter is not present. That makes sense and makes it consistent with other DBAPIHooks (for example MySQL https://airflow.apache.org/docs/apache-airflow-providers-mysql/stable/connections/mysql.html) . Would you mind also updating the docs for it ?

And yeah. That would indeed be nicer URL in many cases:

host:port/service

rather than

host:port?__extra__service_name=service

But for backwards compatibility we should support both

[malthe]

@potiuk just a quick question – the Oracle-specific fields in the connection documentation (e.g. "Sid" and "Service_name"), those don't make sense to me at all looking at:

airflow/airflow/www/forms.py

Line 212 in df143ae

class ConnectionForm(DynamicForm):

Is this simply out of date?

[malthe]

@potiuk I have cleaned up the docs and code to emphasize that Host and Schema (and the rest of the standard fields) are perfectly good for most users.

If a user has an Oracle DSN (Data Source Name) then they can leave Host empty and use the dsn extra.

[potiuk]

Love it!

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

[malthe]

@potiuk so this still needs two additional reviews is that how the process works?

[potiuk]

No. I t just needs fixes of the failing tests (just make sure to rebase it to latest master and make sure all the checks are green)

[malthe]

@potiuk I think the test failure is unrelated. I noticed this in the test log:

Yarn requires Node.js 4.0 or higher to be installed.

[github-actions]

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

[malthe]

These test failures seem unrelated to this PR.

[potiuk]

Yeah. Can you please rebase on top of the latest master? There were a couple of problems in master that have been just fixed.

[malthe]

@potiuk done!

[malthe]

@potiuk fixed – thanks for the patience!

(I had a missing newline in the rst markup.)

[malthe]

@potiuk fixed whitespace – using black locally helps :-)

[potiuk]

Actually having pre-commit installed locally helps even more :)

[boring-cyborg]

Awesome work, congrats on your first merged pull request!