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

[RFC] Allow multiple databases #60

Closed
gildonei opened this issue Jul 23, 2019 · 22 comments
Closed

[RFC] Allow multiple databases #60

gildonei opened this issue Jul 23, 2019 · 22 comments
Assignees
Labels
accepted For accepted RFCs (Requests For Changes) Implemented Implemented RFC new feature For new features

Comments

@gildonei
Copy link
Contributor

Summary

Allow framework to use multiple databases.

Motivation

Some projects may need to utilize different database connections on same project, so, Ubiquity should allow to configure a "default" database and extra connections.

Compatibility with Ubiquity's philosophy

Indicate compatibility or improvements in:

  • ease of use
  • flexibility

Expected results

Be able to connect to different databases

Additional context

Models must be able to specify wich database will be use. If none was specified, it uses default database connection

config.php

return array(
	"siteUrl"=>"http://ubiquity.local",
	"database"=>array(
		"default" => array(
			"type"=>"mysql",
			"dbName"=>"database_name",
			"serverName"=>"0.0.0.0",
			"port"=>3306,
			"user"=>"database_user",
			"password"=>"database_pswd",
			"options"=>array(),
			"cache"=>false
		),
		"extra1" => array(
			....
		),
	),
...
@gildonei gildonei added the new feature For new features label Jul 23, 2019
@jcheron
Copy link
Contributor

jcheron commented Jul 24, 2019

For the moment, the connection to the database is done by:
DAO::startDatabase($config) in services.php

It is also possible to do this in a controller by:
DAO::startDatabase(Startup::$config)

What do you think about the following approach?

With only one database

  • nothing is changed.

With several databases configured as you propose

DAO::startDatabase($config) or DAO::startDatabase($config,'default') starts the default database
DAO::startDatabase($config,'extra1') starts the extra1 database

Note

I prefer to leave this explicit startup, rather than associate a Model class with a database connection.

This choice excludes the possibility of querying data from 2 databases on the same server

@gildonei
Copy link
Contributor Author

gildonei commented Jul 24, 2019

What do you think about the following approach?

I think this works fine, but, I will need to start specific database every time I need to do some operation with those databases.
Imagining that I have a database for log operationsand other to system, I will need to copy and paste startDatabase a lot of times. If i define it inside the model I will have only one place to change, and I will be able to use this model on every controller I need.

I think is a model responsability to know wich database it has to connect before query. Also a model doesn't need to connect to database, it could use a Webservice or other way to retrieve/save data, so you could have a model that use a Rest API to perform CRUD operations, and in this case your "property useDatabase" could be set to false. Plus inside your controller you also would be able to change connection before query using $model->setDatabaseConfig([....]);

@jcheron
Copy link
Contributor

jcheron commented Jul 24, 2019

if the model is responsible for knowing which database to use, it may be considered to do so in this way:

namespace models;

/**
 * @database("extra1")
*/
class User{
}
namespace models;

// Use default database
class Products{
}

In this case, the connection must be made automatically as soon as a model is requested, without the developer having to make an explicit call to DAO::startDatabase

//Starts extra1 connection for the first time
$users=DAO::getAll(User::class);

//Re-use extra1 connection
DAO::update($user);

//Starts default connection
$product=DAO::getById(Product::class,1);

I think it's more practical, but it can have a slight impact on performance (to be benchmarked).

@gildonei
Copy link
Contributor Author

I like the proposed implementation. I think it continues transparent with the @database declaration and developers still can change database in controller when needed.

Now, is to run bench tests to see it is viable.

@jcheron
Copy link
Contributor

jcheron commented Jul 25, 2019

Good News
it seems that the add-on of the multi-database functionality (on multi-db branch) is only a negligible impact on performance:

With TechEmpower Multiple database queries benchmark (with 20 queries):
image

There is a little more discrepancy with my test procedure (but it solicits more the ORM on the relationship and multi-table aspects).
But the 4 ms of difference are insignificant.
image

Now we have to integrate this in the Models part of the Webtools, which should allow us to navigate between the databases.

This feature will be integrated in Ubiquity 2.3.0

@jcheron jcheron added the accepted For accepted RFCs (Requests For Changes) label Jul 25, 2019
@gildonei
Copy link
Contributor Author

Good news and a great job as always.

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

Hi @gildonei ,
the multi-db functionality is ready to be tested:
You have to pass the devtools on the multi-db branch:

composer global require phpmv/ubiquity-devtools:dev-multi-db

You can then create a new project integrating multi-db:

Ubiquity new test-multi -a

The addition of a connection can be done by webtools, provided that the default connection is already operational:

image

@gildonei
Copy link
Contributor Author

gildonei commented Jul 29, 2019

The button to add new connection requested the URL, but form wasn't displayed at the page

image

Also, creating a model to a table on default database, generate an error on cache because all table names are lowercase and cache generation is considering first letter uppercase

Message : Uncaught PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dsv_sigecon.Categoria' doesn't exist in

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

I think it's a versioning problem somewhere.

Can you check that it's consistent with that, with the creation of a new project?

image

@gildonei
Copy link
Contributor Author

Do you want to run composer install ? (y/n)
y
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 12 installs, 0 updates, 0 removals
  - Installing phpmv/ubiquity (dev-multi-db b9648d3): Cloning b9648d3ba7 from cache
  - Installing psr/log (1.1.0): Downloading (100%)
  - Installing monolog/monolog (1.24.0): Loading from cache
  - Installing phpmv/ubiquity-webtools (dev-multi-db 59149a0): Cloning 59149a0acd from cache
  - Installing symfony/polyfill-ctype (v1.11.0): Loading from cache
  - Installing symfony/polyfill-mbstring (v1.11.0): Loading from cache
  - Installing twig/twig (v2.11.3): Loading from cache
  - Installing frameworks/jquery (2.1.4): Loading from cache
  - Installing phpmv/php-mv-ui (2.2.17): Downloading (100%)
  - Installing mindplay/annotations (1.3.1): Loading from cache
  - Installing phpmv/ubiquity-dev (0.0.2): Downloading (100%)
  - Installing czproject/git-php (v3.17.0): Loading from cache
monolog/monolog suggests installing graylog2/gelf-php (Allow sending log messages to a GrayLog2 server)
monolog/monolog suggests installing sentry/sentry (Allow sending log messages to a Sentry server)
monolog/monolog suggests installing doctrine/couchdb (Allow sending log messages to a CouchDB server)
monolog/monolog suggests installing ruflin/elastica (Allow sending log messages to an Elastic Search server)
monolog/monolog suggests installing php-amqplib/php-amqplib (Allow sending log messages to an AMQP server using php-amqplib)
monolog/monolog suggests installing ext-amqp (Allow sending log messages to an AMQP server (1.0+ required))
monolog/monolog suggests installing ext-mongo (Allow sending log messages to a MongoDB server)
monolog/monolog suggests installing mongodb/mongodb (Allow sending log messages to a MongoDB server via PHP Driver)
monolog/monolog suggests installing aws/aws-sdk-php (Allow sending log messages to AWS services like DynamoDB)
monolog/monolog suggests installing rollbar/rollbar (Allow sending log messages to Rollbar)
monolog/monolog suggests installing php-console/php-console (Allow sending log messages to Google Chrome)
phpmv/php-mv-ui suggests installing components/jqueryui (~1.11)
phpmv/php-mv-ui suggests installing twitter/bootstrap (~3.3)
phpmv/php-mv-ui suggests installing semantic/ui (~2.2.1)
Writing lock file
Generating autoload files
     ╭───────────────────────────────────────────────────────────────────────────────╮
     │      ■ info : cache initialization                                            │
     │      · cache directory is C:\wamp64\www\ubiquity\test-multi\app\cache\        │
     ╰───────────────────────────────────────────────────────────────────────────────╯
     ╭────────────────────────────────────────────────────────╮
     │      ■ success : new-project                           │
     │      · project test-multi successfully created.        │
     ╰────────────────────────────────────────────────────────╯

@gildonei
Copy link
Contributor Author

Another strange thing is I lost the content of Models Admin Page after pressed button to generate all models. After that, models page got blank and didn't load anything

image

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

Can you tell me exactly what you do after the project is created:

  • cd test-multi
  • Ubiquity serve
  • ...

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

It's okay, I reproduced your bug:

You can't create a new connection if for the default connection, the models are not generated, and the cache is not initialized (the new connection button should have been disabled)

@gildonei
Copy link
Contributor Author

gildonei commented Jul 29, 2019

a) cd test-multi
b) Ubiquity serve
c) Went to Chrome and run http://127.0.0.1:8090/Admin
d) Tried to add a new database (got "error" above)
d.1) Add a model
d.2) Tried to generate cache (got error about name uppercase)
d.3) Tried to add new database, same error, page content didn't displayed at the page

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

On your screenshot, the models cache is not generated for default connection

The default connection must be ok before creating a new connection.

@gildonei
Copy link
Contributor Author

I just have default connection, couldn't add any other. Also tried do delete (from windows explorer) all cache files, but models page keep blank.

@gildonei
Copy link
Contributor Author

Solved model blank error page. It was my mistake. I add an annotation to specify table name and it had an double arrow "==>) causing error

@gildonei
Copy link
Contributor Author

Done!
After solved blank page problem, and add model and cache for default database, I got able to add a new database connection.

I don't think is necessary to create Models inside a subfolder, as the database name is explicity declared at the model.

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

I don't think is necessary to create Models inside a subfolder, as the database name is explicity declared at the model.

I don't understand this remark.

You mean there's no point in creating the class
Bar in the namespace foo, either foo\Bar,
since the @database('foo') annotation is already in the Bar class?

But you can have the same class (table) in 2 different databases!

@gildonei
Copy link
Contributor Author

But you can have the same class (table) in 2 different databases!

Understood why you decided to use subfolder. I agree with that. This way you can use the namespaces to have 2 classes with same name, but in different namespaces.

I think that now, we can close this issue and wait to integrate it to the next release.

@jcheron
Copy link
Contributor

jcheron commented Jul 29, 2019

It also remains to implement acceptance and unit tests,
and to document all this... before 2.3.0

@gildonei
Copy link
Contributor Author

OK, I will try to help with docs

@jcheron jcheron added Implemented Implemented RFC and removed to test labels Aug 1, 2019
@jcheron jcheron closed this as completed Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted For accepted RFCs (Requests For Changes) Implemented Implemented RFC new feature For new features
Projects
None yet
Development

No branches or pull requests

2 participants