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

API stub #12

Merged
merged 11 commits into from
Dec 2, 2024
Merged

API stub #12

merged 11 commits into from
Dec 2, 2024

Conversation

lmazz1-dbt
Copy link
Collaborator

@lmazz1-dbt lmazz1-dbt commented Nov 29, 2024

Context

Changes proposed in this pull request

  • Added FastAPI dependency
  • Set up and tested healthcheck endpoint
  • Set up and tested one endpoint that talks to the backend (which counts stored entities, for testing)
  • Created stubs for all other endpoints needed by the backend

Guidance to review

I'd like some feedback on the tests, in particular the one for the endpoint talking to the backend. I'm currently setting up the DB and filling it up at the highest level ("link"), but that takes time, and tests something irrelevant from the point of view of the API, and difficult to predict in terms of the output.
On the other hand, we might want to check that the API does the correct backend calls...

I'm also unsure about whether we're gaining much from the FastAPI construct for dependency injection, which I used to inject the backend.

Final point, the ticket suggested implementing this

├── healthcheck
├── testing
│   ├── count
│   │   └── GET, entity={name}  # all MatchboxDBAdapter countable attributes
│   └── clear
│       └── POST                # MatchboxDBAdapter.clear
├── sources
│   ├── GET                     # MatchboxDBAdapter.datasets.list
│   ├── POST                    # MatchboxDBAdapter.index
│   └── {hash}
│       └── GET                 # MatchboxDBAdapter.get_dataset
├── models
│   ├── GET                     # MatchboxDBAdapter.models.list (unimplemented)
│   ├── POST                    # MatchboxDBAdapter.insert_model
│   └── {name}
│       ├── GET                 # MatchboxDBAdapter.get_model (and so model.hash, model.name)
│       ├── DELETE              # MatchboxDBAdapter.delete_model
│       ├── results
│       │   ├── GET             # MatchboxModelAdapter.results getter
│       │   └── POST            # MatchboxModelAdapter.results setter
│       ├── truth
│       │   ├── GET             # MatchboxModelAdapter.truth getter
│       │   └── POST            # MatchboxModelAdapter.truth setter
│       ├── ancestors
│       │   └── GET             # MatchboxModelAdapter.ancestors
│       └── ancestors_cache
│           ├── GET             # MatchboxModelAdapter.ancestors_cache getter
│           └── POST            # MatchboxModelAdapter.ancestors_cache setter
├── query
│   └── GET                     # MatchboxDBAdapter.query
├── validate
│   └── hash
│       └── GET                 # MatchboxDBAdapter.validate_hashes
└── report
    └── models
        └── GET                 # MatchboxDBAdapter.get_model_subgraph

However, I'm instead posting to models/{name} and sources/{hash} when creating new items.

Checklist:

  • My code follows the style guidelines of this project
  • New and existing unit tests pass locally with my changes

Copy link
Collaborator

@wpfl-dbt wpfl-dbt left a comment

Choose a reason for hiding this comment

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

Shaping up nicely!

Could you add the (corrected) ASCII API structure to the PR comment for reference please? Just something to say "this is what I'm aiming to make" for the reviewer to bounce off. Indeed, I've flagged some stuff that having this exact comparison to hand very much helped with.

I think that the principal advantage of Depends for us would be connection sharing between endpoints. Have you looked at the documentation?

Would docstrings for each endpoint help here? I think it might be useful as things get more complex.

src/matchbox/server/api.py Show resolved Hide resolved
src/matchbox/server/api.py Show resolved Hide resolved
src/matchbox/server/api.py Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@wpfl-dbt wpfl-dbt left a comment

Choose a reason for hiding this comment

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

Happy!

@lmazz1-dbt lmazz1-dbt merged commit 22ea503 into main Dec 2, 2024
5 checks passed
@lmazz1-dbt lmazz1-dbt deleted the feature/api branch December 2, 2024 14:31
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