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

Account for change in alldocs field name from type to _type_and_ancestors #851

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

sujaypatil96
Copy link
Collaborator

@sujaypatil96 sujaypatil96 commented Dec 18, 2024

In this PR we are updating the logic for the /data_objects/study/{study_id} endpoint to use the newly renamed field name in alldocs called _type_and_ancestors (which previously used to be called type).

Details

...

Related issue(s)

Fixes #850

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 tested these changes by...

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")

@sujaypatil96 sujaypatil96 marked this pull request as ready for review December 18, 2024 23:51
@aclum
Copy link
Contributor

aclum commented Dec 19, 2024

I would like to see this accompanied by a test to guard against future changes. #817 appears to the PR that introduced this problem where I would have expected a failed test. I did see that @eecavanna made some comments recently #725

Also since the endpoint is not working as expected we should discuss if this should be a hotfix. I would prefer this instead of waiting until the next monthly release cc @pkalita-lbl @shreddd

@shreddd
Copy link
Collaborator

shreddd commented Dec 19, 2024

@sujaypatil96 - can you add a test to ensure that we have a way to verify that the endpoint is working moving forward?

@shreddd
Copy link
Collaborator

shreddd commented Dec 19, 2024

NVM - I see the testing being addressed in #725

@eecavanna eecavanna changed the title Change in alldocs field name from type to _type_and_ancestors Account for change in alldocs field name from type to _type_and_ancestors Dec 19, 2024
@eecavanna
Copy link
Collaborator

In case y'all want to merge in the tests I have written already (they are in branch 725, which is part of draft PR 849) and build on top of those in this branch, that would be OK with me. One way you could do that is by editing PR 849 so that its "base branch" is this issue-850 one, instead of main.

@sujaypatil96
Copy link
Collaborator Author

I was thinking the same thing @eecavanna. I'll help add more test cases to PR #849 today. We can edit the "base branch" to be issue-850, make sure all the tests are passing and merge it in.

Copy link
Collaborator

@dwinston dwinston left a comment

Choose a reason for hiding this comment

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

merging to update base of #849

@dwinston dwinston merged commit 410856f into main Dec 19, 2024
@eecavanna
Copy link
Collaborator

eecavanna commented Dec 19, 2024

Side note: My git client is not showing the original branch (issue-850) as having been merged into main; yet it does show that commit (410856f) as having been merged into main.

Hi @dwinston, did you by any chance merge these changes in with a "squash" commit (via something other than the GitHub UI)?

@sujaypatil96 sujaypatil96 deleted the issue-850 branch December 30, 2024 23:43
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.

/data_objects/study/{study_id} endpoint does not return all of the data objects it used to
5 participants