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

frontend: Refetch resource document after updating #985

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented Dec 13, 2024

What this PR does

Noticed while testing Tags field patching that the PATCH response did not contain the updated Tags. I had to GET the resource again to see the correct set of Tags.

Jira: During testing of ARO-10911 - API: HCP supports managed identities, though not really related
Link to demo recording:

Special notes for your reviewer

Noticed while testing Tags field patching that the PATCH response
did not contain the updated Tags. I had to GET the resource again
to see the correct set of Tags.
@mbarnes mbarnes force-pushed the tags-patch-response branch from 1c24008 to 8716a16 Compare December 13, 2024 15:49
@mociarain
Copy link
Collaborator

That seems broken and is very very annoying.

  • I assume that conditionally re-fetching just on the tags update is too much work but did you consider adding some note to highlight that we could stop re-fetching once this is fixed? Or have you just lost faith and want to re-fetch every time for certainty?
  • Do we have some way to raise this issue back with MSFT?

@mbarnes
Copy link
Collaborator Author

mbarnes commented Dec 16, 2024

It's not broken, it's just the way we use optimistic concurrency control in Cosmos DB.

When we want to update an existing container item, we:

Now here's where the bug comes in. When Cosmos DB creates or updates an item, by default it does not return the updated item content in the response. You have to explicitly request that per-operation through an option named EnabledContentResponseOnWrite.

In almost all cases we don't need the response content, so we leave the option false. Here, however, is the one and only case so far where we do need the response content, because we're expected to return the updated tags in our own response content. Problem is we don't expose the EnabledContentResponseOnWrite option (or indeed any options) through our current DBClient abstraction interface. So easiest solution here is just refetch the item content after the update.

Is it the best solution? Probably not. But since I'm planning to dismantle the DBClient abstraction interface early next year (because it's getting in the way of using Cosmos DB more efficiently; this being one more example), I felt it was a good enough interim solution.

@mociarain
Copy link
Collaborator

Ah yes, the ol'DBClient chestnut. Big +1 to doing something better with that. It's been a pain in Classic too

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