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

Support postgresql connection options #1972

Merged

Conversation

hwc0919
Copy link
Member

@hwc0919 hwc0919 commented Mar 8, 2024

Only support pg now. Because I don't know this feature about other db.

Use a struct to hold db config. Avoid changing function signature everywhere when adding new parameter.

Copy link
Member

@an-tao an-tao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please modify the configuration template in drogon_ctl.

config.example.json Outdated Show resolved Hide resolved
@an-tao an-tao changed the title Support potgresql connection options Support postgresql connection options Mar 9, 2024
@hwc0919 hwc0919 marked this pull request as draft March 9, 2024 09:43
@hwc0919 hwc0919 force-pushed the feature/support-db-connect-options branch from 8c01bc0 to 77c18cc Compare March 17, 2024 07:14
@hwc0919 hwc0919 marked this pull request as ready for review March 17, 2024 07:29
@hwc0919 hwc0919 marked this pull request as draft March 17, 2024 13:37
@hwc0919 hwc0919 force-pushed the feature/support-db-connect-options branch from b53d344 to c1f8f32 Compare March 17, 2024 14:31
@hwc0919 hwc0919 force-pushed the feature/support-db-connect-options branch from 9e4aa60 to 7c022f9 Compare May 10, 2024 06:40
@hwc0919
Copy link
Member Author

hwc0919 commented May 10, 2024

This PR is suspended for the following reason:

DbGeneralConfig is not a elegant solution. We have three types of database, but each one would only use some of the fields.

I want to give each database type its own config struct, but I cannot make a good abstraction out of it.

Say if we have three struct, PostgresConfig, MysqlConfig and SqliteConfig, we have to either change createDbClient() to template method, or extract a base DbConfig struct.

But this is a virtual function in HttpFramework, so extracting a base struct should be the only solution.

However, I found that those three structs having little in common. It would be a meaningless abstraction to extract a base struct.

Then I realize the fundamental cause of the problem is that, we should not use a single createDbClient() method to create three types of db clients in the first place. Now we have to maintain API compatibility, making this PR hard to continue.

@hwc0919 hwc0919 added the help wanted Extra attention is needed label May 10, 2024
@nqf
Copy link

nqf commented May 14, 2024

put config to std::variant . how about ?

auto createDbClient(std::variant<mysql, sqlite, pg> config) {
}

@hwc0919
Copy link
Member Author

hwc0919 commented May 15, 2024

put config to std::variant . how about ?

auto createDbClient(std::variant<mysql, sqlite, pg> config) {
}

I will give it a try

@hwc0919 hwc0919 force-pushed the feature/support-db-connect-options branch 3 times, most recently from 4e4a675 to a980d34 Compare May 20, 2024 15:19
@hwc0919
Copy link
Member Author

hwc0919 commented May 20, 2024

Use std::variant.

Next we should update the DbClient::newXxxClient() and DbClientLockFree api. Or, we could wait.

@hwc0919 hwc0919 force-pushed the feature/support-db-connect-options branch from e177fa8 to 64df11e Compare May 22, 2024 06:56
@hwc0919 hwc0919 marked this pull request as ready for review May 22, 2024 08:31
@hwc0919 hwc0919 requested review from an-tao and marty1885 May 22, 2024 08:39
Copy link
Member

@marty1885 marty1885 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. One minor comment.

@@ -538,9 +538,9 @@ static void loadDbClients(const Json::Value &dbClients)
type.begin(),
[](unsigned char c) { return tolower(c); });
auto host = client.get("host", "127.0.0.1").asString();
auto port = client.get("port", 5432).asUInt();
unsigned short port = client.get("port", 5432).asUInt();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check overflow here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's unnecessary. We didn't check it before, and we didn't check host format. Even if the number was wrong, one could easily find out reason from log.
If we check it, we must store it in uint type, and do convertion in every config construction.

@hwc0919 hwc0919 marked this pull request as draft May 22, 2024 13:30
@hwc0919 hwc0919 marked this pull request as ready for review May 23, 2024 02:43
@an-tao an-tao merged commit 155ae9a into drogonframework:master May 23, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants