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

adds mssql driver #59

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

adds mssql driver #59

wants to merge 7 commits into from

Conversation

FrauElster
Copy link

Summary

Adds a mssql driver in drivers

Changes

  • adds drivers/mssql.go, the actual driver
  • adds drivers/mssql_test.go, these tests are not automated, meaning you have to provide a database and set the tableName, databaseName and connection string. (It is pretty straight forward)
  • adds compiler checks to each driver implementation to verify these implement the Driver interface
  • adds mssql driver to components/ConnectionForm.go
  • adds mssql driver to components/ConnectionSelection.go

Related Issues

Fixes #12

@FrauElster FrauElster marked this pull request as draft June 6, 2024 09:31
@FrauElster
Copy link
Author

Whopsie, forgot a big todo. Give me a moment.

@FrauElster FrauElster marked this pull request as ready for review June 6, 2024 09:48
@jorgerojas26
Copy link
Owner

Thank you for the PR. I'll check and test it a bit a come back with suggestion/merging.

@jorgerojas26
Copy link
Owner

I don't know much about mssql, but i created a docker instance, created a table named "test", i am able to connect to it and list all the "instances" and "tables", but when i select a table it shows the following error:

image

Any idea of why is this happening?

@FrauElster
Copy link
Author

I will check on this as soon as I have time.
I only have an already working remote database and since I am on M1 I cannot really have an local instance.
As I said I will look into this. I am also preparing a PR with some more driver tests if you are interested.

@FrauElster
Copy link
Author

Oh and I did not mention this yet: it's a great project you created here. Props for that. :)

@jorgerojas26
Copy link
Owner

I will check on this as soon as I have time. I only have an already working remote database and since I am on M1 I cannot really have an local instance. As I said I will look into this. I am also preparing a PR with some more driver tests if you are interested.

I'm on M1 also, i just created a docker instance.

Oh and I did not mention this yet: it's a great project you created here. Props for that. :)

Thank you very much, i love this project, i use it daily. I know it is not the best code (first go project), and it have bugs, but with the help of the community we can make this awesome.

Thanks for your time making this PR.

@FrauElster FrauElster closed this Jun 19, 2024
@FrauElster FrauElster reopened this Jun 19, 2024
@FrauElster
Copy link
Author

Okay I found the issue. I specified the database directly in my connection string, therefore the DB-Server knew where to run my queries.
I changed it to explictly use the specified DB.
I also added a logger.

I added some todos, since I did not want to refactor a lot of your code without a review first. And it has nothing to do with the matter of this PR.
My suggestion: All methods of the driver interface should be passed the database. You often hide it in the table with a database.tablename notation.
Further I think all driver methods should accept a context.Context as first argument, as it allows easy control on timeout, or application exit or other things. (https://pkg.go.dev/context)

Check it out, and please give me again feedback. I want to see this project moving forward, so I also can integrated in my day-to-day tools set.

@jorgerojas26
Copy link
Owner

Hi,

above problem is indeed solved, but there's still something i noticed today. When you press "2" inside the table view to see the "Columns", it is empty.

image

Maybe there's a problem in the GetTableColumns function in the driver file.

@FrauElster
Copy link
Author

So the problem is, that even though the drivers interface specifies the GetTableColumns with the parameters database and table, the database is empty, whereas the table is of format database.table.
Apparently I misunderstood the interface, but I think it should be refactored. There should always be a database and a table attribute. The table attribute should never have the database encoded into it. Also I suggest a context.Context is passed, as described in https://pkg.go.dev/context.

I would love to make this a separate PR. For now I will always check if the table param contains a database and if so overwrite database param, I guess.
Change is comming up.

If you have a way for private communication let me know. I would love to here what you say on the interface change. I am sure there is a reason that the database is encoded in the table param, that I just dont know about.

…gnKeys, GetIndexes. Fixes GetTableColumns in mssql driver. adds logger
@jorgerojas26
Copy link
Owner

When i try to filter a table, the following error happens:

image

Apparently I misunderstood the interface, but I think it should be refactored. There should always be a database and a table attribute. The table attribute should never have the database encoded into it. Also I suggest a context.Context is passed, as described in https://pkg.go.dev/context.

I agree that should be refactored

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

Successfully merging this pull request may close these issues.

[feat]: MSSQL Support
2 participants