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

Expose deprecatedById in workflowRuns query interface #300

Merged
merged 4 commits into from
Apr 19, 2024

Conversation

j-x-han
Copy link
Contributor

@j-x-han j-x-han commented Apr 18, 2024

Updates codegen to expose ids of related objects from the same table in the query interface, i.e.:

  • On taxa, allow users to query for taxClassId, taxFamilyId, etc.
  • On workflowRuns, allow users to query for deprecatedById

Manually tested:

  • Update one workflowRun to be deprecatedBy another
Screenshot 2024-04-18 at 4 49 57 PM
  • Verify that the deprecatedById is set as expected, and we can query for it
Screenshot 2024-04-18 at 4 50 42 PM

@j-x-han j-x-han changed the title [WIP] Expose deprecatedById in workflowRuns query interface Expose deprecatedById in workflowRuns query interface Apr 18, 2024
@j-x-han j-x-han marked this pull request as ready for review April 18, 2024 23:54
Comment on lines -628 to -637
params["deprecated_by"] = deprecated_by[0]
del params["deprecated_by_id"]
Copy link
Contributor Author

@j-x-han j-x-han Apr 19, 2024

Choose a reason for hiding this comment

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

I removed these lines because this was causing some unexpected behavior (the actual change is in the codegen template, but this is easier to look at):

  • I have two workflowRuns, one ending in 6397d (status ABORTED) and one ending in d80a8 (status CREATED)
  • In my update mutation, I'm trying to update 6397d to be deprecated by d80a8
  • But after running the mutation, it looks like the reverse has happened, even though the updatedAt timestamp was set on the correct workflowRun
Screenshot 2024-04-18 at 4 03 11 PM Screenshot 2024-04-18 at 4 03 16 PM

Removing these lines allows us to update the correct workflowRun's deprecatedById value as we expect (see the example in the PR description).

To be completely honest, I'm not sure why these lines were causing the incorrect workflowRun to be updated. I tried stepping through the code and it looked like all the correct objects were being fetched/updated with the right params, so I'm not sure why updating the id instead of the actual related object fixes it.

My best guess is that something funky is going on with the underlying logic because deprecatedBy causes workflowRun to have a 1:1 relationship with itself. Let me know if there are concerns with this change or if there are any other solutions to fix this!

Copy link
Collaborator

@rzlim08 rzlim08 left a comment

Choose a reason for hiding this comment

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

LGTM

@j-x-han j-x-han merged commit 8126fda into main Apr 19, 2024
6 checks passed
@j-x-han j-x-han deleted the j-x-han/expose-deprecatedbyid branch April 19, 2024 17:52
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