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

EXPERIMENTAL: Refactor #272

Closed
wants to merge 6 commits into from
Closed

Conversation

rhysrevans3
Copy link
Contributor

Description:
@jonhealy1 I liked what you'd done with the sharing of code between elasticsearch and opensearch in core and wondered if it could be taken further.

I've moved the database specific code to the database logic (only for elasticsearch so far). This removes all database specific code from core.py. If something similar could be done for the other backend this could be move to stac-fastapi. I think other parts like base_database_logic, extensions, models, and utils could also be moved/shared.

Possible benefits:

  • reduced code duplication
  • better separation between API and database layers
  • more cross pollination between backends

I wanted to check this was something positive before I went to far down the rabbit hole.

PR Checklist:

  • Code is formatted and linted (run pre-commit run --all-files)
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable
  • Changes are added to the changelog

@jonhealy1
Copy link
Collaborator

Hi Rhys. I think the idea is to move code out of database logic and into core. This is to make it as easy as possible for someone to implement their own custom backend / database solution. Core is more like database-core.

If you look at one of the original stac-fastapi backends, stac-fastapi-sqlaclchemy is the best example, you can see that there is a fair amount of code that is almost identical to what we use here in core. The idea is to take the common code out of these backends and put them in one place so everything is easier to maintain.

If we move code to database logic from core then at some point we don't need core anymore and then we have reversed everything :) The idea is that only elasticsearch-specific code would be in database_logic. I am sure there is a lot we could do to improve on this idea. Maybe better names too could help for the files.

@jonhealy1
Copy link
Collaborator

I was thinking a little bit about moving this to the core library - https://github.com/stac-utils/stac-fastapi-sqlalchemy - it used to be popular but now it is not even being maintained.

@rhysrevans3
Copy link
Contributor Author

That's a very good point and I agree.

I don't think I put my idea across very well and it's probably better to separate into two points.

  1. The main point, which is sound like you've already been thinking of doing, was to move /core to a core library. Is there a reason why this is stac-fastapi-sqlalchemy and not stac-fastapi
  2. Database level shared code should be in base_database_logic.py (not database logic) rather than core.py. I think this give better the separation between API and database and makes it easier to override for a specific backend if needed. But this is probably more of a design pattern and just a personal preference.

Closing this pull request as I think we're on the same page with the important part 👍

@jonhealy1
Copy link
Collaborator

jonhealy1 commented Jun 18, 2024

Moving shared code to base_database_logic may be a really good idea. I am not sure what you asking about stac-fastapi-sqlalchemy and stac-fastapi.

@jonhealy1
Copy link
Collaborator

jonhealy1 commented Jun 18, 2024

Originally there was just the stac-fastapi repo. That repo had two backends - pgstac and sqlalchemy. Afterwards we created stac-fastapi-elasticsearch which borrowed from stac-fastapi but lived in a separate repo. Later the main developers moved the pgstac and sqlalchemy code out of the main stac-fastapi repo into separate repos. The thing with all of the backends is that they share a lot of the same logic. You may know all of this already.

@jonhealy1
Copy link
Collaborator

The logic that all of the backends share could maybe be moved back to the main stac-fastapi repo someday

@rhysrevans3
Copy link
Contributor Author

Ah I didn't know the history but that explains the shared logic thank you.

I was thinking a little bit about moving this to the core library - https://github.com/stac-utils/stac-fastapi-sqlalchemy - it used to be popular but now it is not even being maintained.

I thought this might have meant that the shared code would live in stac-fastapi-sqlalchemy so wanted to just double check that it would be the main stac-fastapi repo that shared code would be moved too.

@rhysrevans3 rhysrevans3 deleted the refactor branch June 19, 2024 07:16
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