Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MsSql hook does not use extra parameters when creating PymssqlConnection #43798

Open
2 tasks done
ilarionkuleshov opened this issue Nov 7, 2024 · 2 comments · May be fixed by #44190
Open
2 tasks done

MsSql hook does not use extra parameters when creating PymssqlConnection #43798

ilarionkuleshov opened this issue Nov 7, 2024 · 2 comments · May be fixed by #44190

Comments

@ilarionkuleshov
Copy link

Apache Airflow Provider(s)

microsoft-mssql

Versions of Apache Airflow Providers

apache-airflow-providers-microsoft-mssql==3.9.1

Apache Airflow version

2.9.2

Operating System

Ubuntu 22.04

Deployment

Docker-Compose

Deployment details

No response

What happened

I found that MsSqlHook in get_conn method does not use extra parameters from airflow connection when creating PymssqlConnection. Thus, it is impossible to pass any special argument from the extra parameters of the airflow connection to pymssql.connect function.
I discovered this when I needed to pass the tds_version argument, because without it the connection to my database did not occur successfully.
Also, the documentation says that extra parameters can be used in MSSQL connection, but in fact this is not the case.

What you think should happen instead

I think the get_conn method should look something like this:

def get_conn(self) -> PymssqlConnection:
    """Return ``pymssql`` connection object."""
    conn = self.connection
    extra_conn_args = {arg_name: arg_val for arg_name, arg_val in conn.extra_dejson.items() if arg_name != "sqlalchemy_scheme"}  # new code line
    return pymssql.connect(
        server=conn.host,
        user=conn.login,
        password=conn.password,
        database=self.schema or conn.schema,
        port=str(conn.port),
        **extra_conn_args,  # new code line
    )

I tested it locally, and with this code the extra parameters are successfully passed to PymssqlConnection. And in my case the connection to the DB was successful.

I would also like to note that I added an excluding for the sqlalchemy_scheme parameter, because in the hook it is used in the sqlalchemy_scheme property, and in the pymssql.connect function it is not needed.

How to reproduce

To reproduce the bug, you should try to add any extra parameter to the airflow connection. And this will not affect the connection to the database, because the hook does not use extra parameters to create a connection to the database.

Anything else

I could probably create a PR with a fix, but since I'm new here, I assume that there might be a catch, and maybe it was intended that extra parameters not be passed to the pymssql connection (if so I wonder why). So, for now, I decided to just create an issue.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@ilarionkuleshov ilarionkuleshov added area:providers kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Nov 7, 2024
Copy link

boring-cyborg bot commented Nov 7, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@potiuk potiuk added good first issue and removed needs-triage label for new issues that we didn't triage yet labels Nov 11, 2024
@jx2lee
Copy link
Contributor

jx2lee commented Nov 18, 2024

@potiuk Hi, Could i take this? assign to me please ✈️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants