Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

Add Async MySQL support #766

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

vdauchy
Copy link

@vdauchy vdauchy commented May 17, 2021

Hi @mpociot ,

This PR is a follow up to #644 discussion.

In the PR I did:
- Add react/mysql to composer to support async connection to MySql.
- Reformatted the config for database connections: This is a proposition to support a broader, less specific, way to configure DB connections that shall match Laravel's way of configure databases.
- Add singleton on Async MySql connection.
- Add deferred rejection callback in both SQLite and MySql AppManagers, the reason would be able to log errors properly (like connection errors, no table, etc...). Unfortunatelly I did not see any usable upstream logger, maybe it's something missing ?

Testing on a personnal project this all seem to work as expected.

@vdauchy vdauchy force-pushed the async-app-managers branch from a1d9d55 to 5127ef4 Compare May 17, 2021 09:43
@mpociot
Copy link
Member

mpociot commented May 18, 2021

Awesome work on this - thank you so much for the PR!
Can you modify the database configuration array so that it has an array for each driver? Pretty much like what Laravel does. I think this would be the best option and also not confuse users.

Regarding the logger: yeah I don’t think there’s anything available yet, besides logging to CLI.

Can you also add tests for the MySQL connection, like we have for SQLite?

@vdauchy
Copy link
Author

vdauchy commented May 18, 2021

Hi @mpociot,

I corrected the db configuration structure, I removed the the checks on singletons so now both sqlite and mysql singleton are registered everytime.

An other way database configuration could be handled could be by setting the connection name (configured in Laravel's database.php file) instead. That would limit duplication of configurations, and one of the big point I see: Async db connection could be only used on the WS process, for UI since you go through the Laravel app (Nginx, Apache, etc...) you may just use a classic Eloquent model... So both connection may surely be duplicated at the end.

If we even want to push the idea further, we could even just gives a model class in the config and then get the db connection from there, then replace stuff like database->query('SELECT * from `apps` WHERE `secret` = ?', [$appSecret]) by something like: database->query($appModel::query()->whereSecret(null)->toSql(), [$appSecret])...

But that's just brain food... :-)

Regarding the MySQL tests, I will need to dig on MySQL github action I suppose... will be fun :-)

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2021

Codecov Report

Merging #766 (957744d) into async-app-managers (fcb3a53) will decrease coverage by 3.56%.
The diff coverage is 4.76%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##             async-app-managers     #766      +/-   ##
========================================================
- Coverage                 87.09%   83.52%   -3.57%     
  Complexity                    2        2              
========================================================
  Files                        64       65       +1     
  Lines                      1867     1906      +39     
========================================================
- Hits                       1626     1592      -34     
- Misses                      241      314      +73     
Impacted Files Coverage Δ Complexity Δ
src/Apps/MysqlAppManager.php 0.00% <0.00%> (ø) 0.00 <0.00> (?)
src/Apps/SQLiteAppManager.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/WebSocketsServiceProvider.php 71.00% <23.52%> (-11.56%) 0.00 <0.00> (ø)
src/Console/Commands/StartServer.php 88.11% <0.00%> (-0.38%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcb3a53...957744d. Read the comment docs.

@mpociot
Copy link
Member

mpociot commented May 18, 2021

@vdauchy great - thank you!

Hm..I guess it all depends on how flexible we want this to be.
We could also just make a database connection configurable and then pull the settings from the database.php config file.
This would completely avoid duplicating configurations.
Of course, this would only work with supported drivers, but we can list the ones we support out of the box in the comment + documentation.
So maybe this would be even easier and user-friendly.

Providing a model would work as well, but I feel that this might be confusing. Which model should you pick? Because it's not really about the model, but the database connection.

@mpociot mpociot merged commit 437df8b into beyondcode:async-app-managers Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants