Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

SQLAlchemy models, refactor helpers calls #73

Merged
merged 16 commits into from
Dec 9, 2016
Merged

Conversation

amercader
Copy link
Member

@amercader amercader commented Dec 7, 2016

fixes #33


@roll This is ready to review.

There are two main things:

  1. Obviously set up SQLAlchemy models and replace dataset calls with SA equivalents for interaction with the DB
  2. I didn't like how in some places, specially the tasks and the tests we were interacting directly with the DB rather than going through helpers. In most cases plugins or tasks should not need to worry about SA syntax (similarly to how dataset abstracted some of this complexity) and helpers should be the main API to interact with jobs and other future objects. Because tasks use a different session, this implies that sometimes you need to explicitly pass a session to the helper methods. I think the benefits of conceptually separating the interaction with the DB outweigh this minor inconvenience.

Let me know what you think

Drop dataset and use pure SQLAlchemy models.

I'm still not 100% happy with how the tasks don't use the same interface
for getting (and updating) jobs than the rest of the app, just because
they don't use the same session. We can probably pass the session as an
optional parameter to the helpers and use these consistently.
One important thing to note is that the factories explicitly created the
object on the database in all cases. This is to simplify tests where two
sessions are used (tasks). We can revisit this in the future but I don't
think it will be an issue.
Rather than having SQLAlchemy all over the place let's keep these at the
helpers level so plugins or tasks don't need to worry about SA calls.

This implies that sometimes you need to explicitly pass a session if the
one you are using is not the default one one `goodtablesio.services` (ie
you are in a task). I think the benefits of conceptually separating the
interaction with the DB outweigh this minor incovenience.

Improved helpers docstrings.
@amercader amercader added this to the Alpha milestone Dec 7, 2016
@roll
Copy link
Member

roll commented Dec 8, 2016

@amercader
A few thoughts:

  • one things which is bothering me long time is how we run celery tasks - we do it in plugins.github.blueprint and helpers.create - in my opinion it's pretty inconsistent (because it's different logical layers). I think we need to sync it e.g. moving second one to blueprints.api (so all task runs inside web handlers)
  • I don't see anything criminal in exposing SA interface to plugins because it's still our internal code. But I see a good benefit of encapsulating all db related code in models (commonly used data layer encapsulation). WDYT about it? Getting rid of the most of helpers moving db-related code as static methods of models.Job - I suppose it's pretty common pattern. SA specifics could be still hidden here. I suppose our helpers is getting messy already. If we will move code this way we will be having all db-job related code inside e.g. models.job module.

@amercader
Copy link
Member Author

@roll +1 on the first point, see 904ce90

On moving DB logic to models, I'm also happy to do that. I'm not so keen on static methods on classes though, specially on the actual SQLAlchemy models. I think plugins should just pass and get dicts around and not actual SA objects. What about keeping the helpers as they are but moving them to the models.job module as you suggested?:

  • helpers.create_job() -> models.job.create()
  • helpers.update_job() -> models.job.update()
  • helpers.get_job() -> models.job.get()
  • helpers.get_jobs() -> models.job.find()
  • helpers.get_job_ids() -> models.job.get_ids()

Usage:

from goodtablesio import models

job = models.job.create(params)

or this, whatever we prefer:

job = models.job.create(**params)

@roll
Copy link
Member

roll commented Dec 8, 2016

@amercader
I'd be happy to have it in one place. Static methods or function or whatever - just not so important for me than concept of highly cohesive modules. So +1 with any you prefer.

@amercader
Copy link
Member Author

@roll please have a look

I've moved all db related methods to models.job. I think this is much nicer, thanks for your feedback.

While I was at it I renamed Job.job_id to Job.id as it makes more sense. I also fixed some inconsistencies in the models definitions / migrations.

Copy link
Member

@roll roll left a comment

Choose a reason for hiding this comment

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

👍

@amercader amercader merged commit 2d233f8 into master Dec 9, 2016
@amercader amercader deleted the sqlalchemy-models branch December 9, 2016 08:33
@amercader amercader removed the review label Dec 9, 2016
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.

Migrate to SQLAlchemy models
2 participants