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

Document query parameters directly instead of via freeform text blurbs #804

Conversation

eecavanna
Copy link
Collaborator

@eecavanna eecavanna commented Dec 1, 2024

In this branch, I effectively relocated the parameter-level documentation from the freeform blurbs atop two endpoint groups on Swagger UI, to the endpoints themselves in Swagger UI. That applies to the endpoints in the "find" section of the Swagger UI, and the endpoints whose paths begin with /nmdcschema/ in the "metadata" section of Swagger UI.

Details

The main challenge here was figuring out how to properly mix and match FastAPI's and Pydantic's abstractions in order to get descriptions of Pydantic model attributes to appear next to the corresponding parameter fields on the Swagger UI page. There seem to me to be some unresolved upstream issues at the intersection of those three things:

  • Encapsulating query parameters within a Pydantic model
  • Displaying query parameter descriptions on Swagger UI
  • FastAPI's integrations with Pydantic and Swagger/OpenAPI

image

In this PR branch, I landed on an approach that works for the description field, specifically. It does not work for the title and examples fields. In my local experimentation, I was able to get something to work for all fields, but it involved converting the Pydantic models into regular Python classes (that don't inherit from Pydantic's BaseModel class) and that was more of a deviation from the current state of things than I wanted to do in a documentation-focused PR.

I also added lots of TODO comments about adding documentation to functions that lack it (I just did so for functions I dealt with while working on this PR). <soapbox>I think a given piece of code is generally read more times than it is written (i.e. >1 versus 1), spending time deciphering code can take time/energy, and I think the person writing the code is generally best positioned to document their expectations about whatever it is they are implementing</soapbox>, so I plan to create a ticket(s) about the ones of these I think will be most impactful and assign them to the author(s) of those pieces of code.

Finally, here's a screenshot showing query parameter documentation appearing next to the parameters being described. Previously, those query parameters were documented in a freeform text blurb above that group of "metadata" endpoints. The difference shown in this screenshot is representative of the way documentation has changed for many endpoints in this PR branch.

image

Related issue(s)

Fixes #665

Contributes to microbiomedata/docs#24

Related subsystem(s)

  • Runtime API (except the Minter)
  • Minter
  • Dagster
  • Project documentation (in the docs directory)
  • Translators (metadata ingest pipelines)
  • MongoDB migrations
  • Other

Testing

  • I tested these changes (explain below)
  • I did not test these changes

I confirmed all the tests performed by the GitHub Actions workflows still pass.

Documentation

  • I have not checked for relevant documentation yet (e.g. in the docs directory)
  • I have updated all relevant documentation so it will remain accurate
  • Other (explain below)

Maintainability

  • Every Python function I defined includes a docstring (test functions are exempt from this)
  • Every Python function parameter I introduced includes a type hint (e.g. study_id: str)
  • All "to do" or "fix me" Python comments I added begin with either # TODO or # FIXME
  • I used black to format all the Python files I created/modified
  • The PR title is in the imperative mood (e.g. "Do X") and not the declarative mood (e.g. "Does X" or "Did X")

@eecavanna eecavanna self-assigned this Dec 1, 2024
@eecavanna eecavanna marked this pull request as ready for review December 1, 2024 01:35
@eecavanna eecavanna marked this pull request as draft December 1, 2024 01:35
eecavanna and others added 20 commits November 30, 2024 17:41
This involved refactoring the collection name validation code.
When the `@router.get` decoration included the `dependencies` kwarg,
the path parameter's description (defined in the function signature)
would not appear on Swagger UI. An alternative approach _may_ have
been to move the parameter's description (and its other annotations)
up to the `dependencies` kwarg (I did not try this). Since I removed
the `dependencies` kwarg, I implemented the same logic within the
endpoint's request handler function, itself.
@eecavanna eecavanna requested a review from dwinston December 3, 2024 06:47
@eecavanna eecavanna marked this pull request as ready for review December 3, 2024 06:47
@eecavanna
Copy link
Collaborator Author

Hi @dwinston, in case you review this PR branch and have time to address any of the TODOs I added, I'm OK with you making commits directly to this branch.

@eecavanna
Copy link
Collaborator Author

According to the ticket for the next NMDC Release, code destined for that release is due in main by December 9. I would like for these changes to be included in that release.

@eecavanna
Copy link
Collaborator Author

eecavanna commented Dec 5, 2024

I'm planning to merge this in on Thursday afternoon. Monday is the date on which we'll be freezing "what's in the dev environment" so team members can begin testing the code that will be included in the upcoming release.

…ng code

No one uses these endpoints in their still-incomplete form. From the prod analytics (since 2023-10-05):
> db.getCollection("_runtime.analytics").countDocuments({"path": {"$regex": "^/pipeline_search"}}
0

So, remove this cruft.
This endpoint played a role early on in NMDC for a workflow for metadata import. That workflow is no longer used.

No use since 2023-10-05:
> db.getCollection("_runtime.analytics").countDocuments({"path": {"$regex": "^/metadata/json:validate_urls_file"}})
0
@eecavanna
Copy link
Collaborator Author

Thank you, @dwinston, for addressing so many of the TODOs. I think this will all make maintenance of the code base easier for people over time.

@eecavanna
Copy link
Collaborator Author

Thanks again, @dwinston. Merging into main now, ahead of Monday's code freeze.

@eecavanna eecavanna merged commit 55c7f94 into main Dec 6, 2024
2 checks passed
@eecavanna eecavanna deleted the 665B-convert-swagger-tag-metadata-freeform-blurbs-into-parameter-annotations branch December 6, 2024 06:57
@mkshah605 mkshah605 mentioned this pull request Dec 10, 2024
12 tasks
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.

Convert Swagger tag metadata (freeform blurbs) into parameter annotations
2 participants