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

mysql: lua-resty-mysql backend, models support, fix luasql backend, implement entity_exists #250

Merged
merged 6 commits into from May 7, 2015

Conversation

Projects
None yet
3 participants
@starius
Contributor

starius commented Apr 5, 2015

Changes:

  • lua-resty-mysql backend was implemented
  • luasql backend was fixed
    • forgotten argument password is now passed to method connect
    • returned number and boolean valued are converted from strings to Lua types
  • function entity_exists of module mysql.schema was implemented
  • support for models

Support for models:

  • modules "lapis.db" and "lapis.db.schema" select postgres or mysql based on the site's config
  • method Model.create did rely on RETURNING keyword to get ID of new rows. MySQL has no RETURNING keyword. I used a quick workaround:
    • if ID is already known (it is not autoincrement field), than variable "values" doesn't need any changing nothing needs to be changed in variable "values".
    • otherwise, ID is considered to be autoincrement field. The new row ID is provides by both mysql backends: luasql provides res.last_auto_id and lua-resty-mysql provides res.insert_id. Both of them are checked and used as value for the key @primary_key on success. This trick breaks code isolation between models and backends, so it needs some refactoring.

All the changes were tested with my project kodomoquiz, which now supports mysql.

see #46

@leafo

This comment has been minimized.

Show comment
Hide comment
@leafo

leafo Apr 8, 2015

Owner

nice, thanks.

model_spec.moon fails now because it requires lapis.db.model, which requires lapis.db and the test environment has no configuration unless it's explicitly been added.

Error → ./lapis/db/model.moon @ 2
./spec/model_spec.moon
./lapis/db/model.moon:2: You have to configure either postgres or mysql

I think we should probably create a model base class and and have the postgres and mysql versions extend from it. lapis.db.model would then use the same system you added to lapis.db. There are are going to be more divergences in implementation other than the primary key thing.

This way model spec can be updated to require the specific implementation of the model.

Owner

leafo commented Apr 8, 2015

nice, thanks.

model_spec.moon fails now because it requires lapis.db.model, which requires lapis.db and the test environment has no configuration unless it's explicitly been added.

Error → ./lapis/db/model.moon @ 2
./spec/model_spec.moon
./lapis/db/model.moon:2: You have to configure either postgres or mysql

I think we should probably create a model base class and and have the postgres and mysql versions extend from it. lapis.db.model would then use the same system you added to lapis.db. There are are going to be more divergences in implementation other than the primary key thing.

This way model spec can be updated to require the specific implementation of the model.

@leafo

This comment has been minimized.

Show comment
Hide comment
@leafo

leafo Apr 8, 2015

Owner

I fixed travis as well so you might want to rebase if you want it to run the tests on this branch

Owner

leafo commented Apr 8, 2015

I fixed travis as well so you might want to rebase if you want it to run the tests on this branch

starius added some commits Apr 5, 2015

db.mysql, luasql: convert numbers and bools to Lua
Method cur:fetch of luasql-mysql returns a table, which values
are strings, according to [1]. Numbers and boolean variables can
be converted to Lua, which was done by this commit. Information
about names and types of columns is provided by methods
cur:getcolnames() and cur:getcoltypes(),

see #46

[1]: https://keplerproject.github.io/luasql/doc/us/manual.html#cur_fetch
mysql: support for models
 * modules "lapis.db" and "lapis.db.schema" select postgres
   or mysql based on the site's config
 * method Model.create did rely on RETURNING keyword
   to get ID of new rows. MySQL has no RETURNING keyword.
   I used a quick workaround:

   - if ID is already known (it is not autoincrement field),
     than variable "values" doesn't need any changing
     nothing needs to be changed in variable "values".
   - otherwise, ID is considered to be autoincrement field.
     The new row ID is provides by both mysql backends:
     luasql provides res.last_auto_id and lua-resty-mysql
     provides res.insert_id. Both of them are checked and used
     as value for the key @primary_key on success. This trick
     breaks code isolation between models and backends, so
     it needs some refactoring.

All the changes were tested with my project kodomoquiz [1],
which now supports mysql.

see #46

[1]: https://github.com/starius/kodomoquiz
@lordnynex

This comment has been minimized.

Show comment
Hide comment
@lordnynex

lordnynex Apr 8, 2015

👍 for base model.

I'd love to get mongo/redis/cassandra 'providers' integrated. This is currently not possible because the current model class is strongly tied to postgres. :)

lordnynex commented Apr 8, 2015

👍 for base model.

I'd love to get mongo/redis/cassandra 'providers' integrated. This is currently not possible because the current model class is strongly tied to postgres. :)

@starius

This comment has been minimized.

Show comment
Hide comment
@starius

starius Apr 8, 2015

Contributor

Thank you for fixing travis! Current build is failing.

I think we should probably create a model base class and and have the postgres and mysql versions extend from it.

👍

  1. What code creates and setups database when running tests? I can't find anything like "apt-get install postgres" and setup commands.
  2. Should spec/model_spec.moon use postgres or mysql? What is the difference between spec_postgres/model_spec.moon and spec/model_spec.moon? I want to avoid code duplication in tests. Common code (models, asserts) should be moved to some module. Tests spec_postgres/model_spec.moon and spec_mysql/model_spec.moon should be thin modules, that set database to "postgres" or "mysql" and load this shared module.
Contributor

starius commented Apr 8, 2015

Thank you for fixing travis! Current build is failing.

I think we should probably create a model base class and and have the postgres and mysql versions extend from it.

👍

  1. What code creates and setups database when running tests? I can't find anything like "apt-get install postgres" and setup commands.
  2. Should spec/model_spec.moon use postgres or mysql? What is the difference between spec_postgres/model_spec.moon and spec/model_spec.moon? I want to avoid code duplication in tests. Common code (models, asserts) should be moved to some module. Tests spec_postgres/model_spec.moon and spec_mysql/model_spec.moon should be thin modules, that set database to "postgres" or "mysql" and load this shared module.
@leafo

This comment has been minimized.

Show comment
Hide comment
@leafo

leafo Apr 9, 2015

Owner

There are three test folders, spec spec_mysql and spec_postgres. The spec directory has generic lapis tests, any of the model/db specs in there don't actually run queries on a database but instead generate SQL to be checked by an assertion to be a certain string.

I added spec_mysql and spec_postgres as I started writing specs that would touch an actual server. There's nothing that automates starting the server. Travis currently doesn't run these specs. There are two makefile commands to set up the databases for each of these spec directories, make test_db for postgres, and make mysq_test_db for mysql. I do want to have these on travis, I haven't gotten an opportunity to set it up yet.

Regarding spec/model_spec.moon, currently that only tests query generation for postgres, since that's all there was before this patch. When we do this base class refactor I would just update that spec to test using the postgres model class first, just so it continues to test what it originally tested. Afterwards we can figure out how to split things up and add mysql tests in there.

Owner

leafo commented Apr 9, 2015

There are three test folders, spec spec_mysql and spec_postgres. The spec directory has generic lapis tests, any of the model/db specs in there don't actually run queries on a database but instead generate SQL to be checked by an assertion to be a certain string.

I added spec_mysql and spec_postgres as I started writing specs that would touch an actual server. There's nothing that automates starting the server. Travis currently doesn't run these specs. There are two makefile commands to set up the databases for each of these spec directories, make test_db for postgres, and make mysq_test_db for mysql. I do want to have these on travis, I haven't gotten an opportunity to set it up yet.

Regarding spec/model_spec.moon, currently that only tests query generation for postgres, since that's all there was before this patch. When we do this base class refactor I would just update that spec to test using the postgres model class first, just so it continues to test what it originally tested. Afterwards we can figure out how to split things up and add mysql tests in there.

model base class
Rename "model" to "base_model", extend it with "postgre.model"
and "mysql.model" classes. Methods "create" and "update"
are implemented in descendant classes only.

Test "model_spec" was updated. Currently it changes config
of Lapis so that postgres is used.

see #250
see #46
@starius

This comment has been minimized.

Show comment
Hide comment
@starius

starius Apr 15, 2015

Contributor

Base class for models added

Contributor

starius commented Apr 15, 2015

Base class for models added

@leafo leafo merged commit cd3e7d2 into leafo:master May 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leafo

This comment has been minimized.

Show comment
Hide comment
@leafo

leafo May 7, 2015

Owner

Sorry for the delay. It's merged now. I went through a made some refactors and pulled in the model specs from postgres into spec_mysql.

I made db a parameter of the model now. Since in the tests there isn't a config, and the models were depending on requiring lapis.db, it was failing. Now the models can just reference db stored on self to get the correct version based on the database.

I renamed lua_resty_mysql to resty_mysql.

There was a bug with composite primary keys messing up autogen ids that I fixed.

I still haven't had a chance to get tests hitting the resty_mysql backend, but I wanted to do some refactors back in master to make setting up test environments a bit easier. After I get that working, and tests written, I'll start updating the documentation.

Thanks for the patch!

Owner

leafo commented May 7, 2015

Sorry for the delay. It's merged now. I went through a made some refactors and pulled in the model specs from postgres into spec_mysql.

I made db a parameter of the model now. Since in the tests there isn't a config, and the models were depending on requiring lapis.db, it was failing. Now the models can just reference db stored on self to get the correct version based on the database.

I renamed lua_resty_mysql to resty_mysql.

There was a bug with composite primary keys messing up autogen ids that I fixed.

I still haven't had a chance to get tests hitting the resty_mysql backend, but I wanted to do some refactors back in master to make setting up test environments a bit easier. After I get that working, and tests written, I'll start updating the documentation.

Thanks for the patch!

starius added a commit to starius/wordpress-lapis that referenced this pull request Jun 5, 2015

mysql backend: lua_resty_mysql -> resty_mysql
It was called lua_resty_mysql in the pull request [1] and was
renamed to resty_mysql in lapis release.

[1] leafo/lapis#250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment