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

DB Client #21

Merged
merged 17 commits into from
Mar 4, 2024
Merged

DB Client #21

merged 17 commits into from
Mar 4, 2024

Conversation

SerRichard
Copy link
Member

@SerRichard SerRichard commented Feb 23, 2024

Add layout for how to initialize connection to postgresql. Functional for both a live DB connection and also for the tests. The tests require postgresql to be installed where they are running. May need to add to the .devcontainer for the future.

@SerRichard
Copy link
Member Author

From the models, it works with a new DB connection and can initialize the tables, but I'm not sure how it behaves on an already existing database

@geospatial-roman
Copy link
Collaborator

From the models, it works with a new DB connection and can initialize the tables, but I'm not sure how it behaves on an already existing database

Are you talking about the session.add() behaviour for existing tables / users (in this case) here? maybe this doc helps :

Session.add() is used to place instances in the session. For transient (i.e. brand new) instances, this will have the effect of an INSERT taking place for those instances upon the next flush. For instances which are persistent (i.e. were loaded by this session), they are already present and do not need to be added. Instances which are detached (i.e. have been removed from a session) may be re-associated with a session using this method

Copy link
Collaborator

@geospatial-roman geospatial-roman left a comment

Choose a reason for hiding this comment

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

lgtm

@SerRichard
Copy link
Member Author

SerRichard commented Feb 29, 2024

@geospatial-roman

Are you talking about the session.add()

I'm not sure, the test case I feel like is missing would be the following.

from openeo_fast import User

EODCUser(User):
   # Extend model with some extra columns

this is fine and one of the unit tests checks that this works as expected

Though, if I have already previously initialized the application and applied version 1 of the models to the database, but now, version 2 is released and there is a new column to the user, I am not sure that this setup will revise the database in the way that would be expected (which is what we previously used alembic for). Not sure if that makes the mess in my head any clearer?

I'll have a review of the documentation and see if I can come up with something, but, yeah, right now, not sure this sufficiently replaces the revisions that are provided by alembic.

edit -- did not realise alembic actually allows for an auto generate. So on the client side, we can use the models to auto-generate the relevant alembic revisions, I think this needs to be done client side, to handle any extensions we might independently add to the models. I'll try to encapsulate this in a test and add some documentation.

@geospatial-roman
Copy link
Collaborator

geospatial-roman commented Feb 29, 2024

edit -- did not realise alembic actually allows for an auto generate. So on the client side, we can use the models to auto-generate the relevant alembic revisions, I think this needs to be done client side, to handle any extensions we might independently add to the models. I'll try to encapsulate this in a test and add some documentation.

Ah I see, yeah for changes to the DB models I think sticking to alembic and auto generate the revisions makes sense. If later revisions of the table add required columns however this might lead to conflicts, if the according data entries to be added/migrated do not have the required new column. But if we keep a simple data model, like the User in here for the first revisions and have according test data, we can add revisions specific to our deployments outside of this repo.

@SerRichard
Copy link
Member Author

Updated it and added tests to check for this expected behaviour with alembic.

So, testing framework will auto-generate and apply the revisions based on the BASE from the client settings. Test also checks these can be extended and applied in-situ by a user. Should be fine now, just need to document the expectation for how to use it so it's clear!

@SerRichard SerRichard merged commit c7dea5c into main Mar 4, 2024
3 checks passed
@SerRichard SerRichard deleted the create-db-client branch March 4, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants