Skip to content

Allows non ancestor queries inside a transaction - #366

Merged
copybara-service[bot] merged 4 commits into
GoogleCloudPlatform:mainfrom
mmbsoftware-it:main
May 5, 2025
Merged

Allows non ancestor queries inside a transaction#366
copybara-service[bot] merged 4 commits into
GoogleCloudPlatform:mainfrom
mmbsoftware-it:main

Conversation

@step76

@step76 step76 commented Apr 24, 2025

Copy link
Copy Markdown
Contributor

Hello,

As stated here, the new Firestore in Datastore mode allows executing non-ancestor queries inside a transaction. The library enforces the previous behaviour, so these changes allow leveraging the new feature using the legacy bundle.

This could be useful for many people still using the legacy bundle.

step76 added 4 commits April 15, 2025 17:33
the Firestore in Datastore mode allows to perform queries without an
ancestor inside a transaction
…ction

the new Firestore in Datastore mode allows not ancestor queries in
transaction so the local datastore is changed to accept this behaviour
@ludoch

ludoch commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator

Anyway we can add a test via the local appengine datastore dev emulator?
Is it only a client library change or do we need to change the dev emulation as well?

@step76

step76 commented Apr 25, 2025

Copy link
Copy Markdown
Contributor Author

Anyway we can add a test via the local appengine datastore dev emulator? Is it only a client library change or do we need to change the dev emulation as well?

I also changed the emulator. The change was pretty trivial. There was a point where the code assumed there was an ancestor in a transaction context, trusting the checks done in the library, so I changed that check to separate the transaction logic and the ancestor logic. The emulator implementation remained the same.

@maigovannon

maigovannon commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator

Maybe I am way off here...but doesn't this change enforce the firestore in datastore mode for all callers? What about the clients who are using in just plain datastore calls? Shouldn't they still mandate enforcing Ancestor queries? @ludoch

@step76

step76 commented Apr 25, 2025

Copy link
Copy Markdown
Contributor Author

Maybe I am way off here...but doesn't this change enforce the firestore in datastore mode for all callers? What about the clients who are using in just plain datastore calls? @ludoch

As far as I know all the datastore instsances were automatically migrated to the new Firestore in Datastore mode.

@ludoch

ludoch commented Apr 25, 2025

Copy link
Copy Markdown
Collaborator

Thanks, just so that I have all the context, where "I also changed the emulator...." did you do the change?
Google3? CL?

@step76

step76 commented Apr 26, 2025

Copy link
Copy Markdown
Contributor Author

Thanks, just so that I have all the context, where "I also changed the emulator...." did you do the change? Google3? CL?

I'm talking about the local dev server contained in this project. One of the three files I've changed in this PR i about the local dev server.

@ludoch ludoch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approval, but still need to wait for extra internal tests done inside Google code base.

@ludoch

ludoch commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator

We have more internal tests still using TRANSACTION_REQUIRES_ANCESTOR that are now failing, so we also need to propagate this change internally,
Sorry about that, it will take a few cycles on our side, and we might have to clone this internally before doing the gh change.
Thanks for you contribution!

@copybara-service
copybara-service Bot merged commit 83faf79 into GoogleCloudPlatform:main May 5, 2025
@ludoch

ludoch commented May 5, 2025

Copy link
Copy Markdown
Collaborator

After minor internal adaptation, this PR is now back to gh repository.
I will initiate a Cloud CLI update so the release can be in new Cloud CLI Tuesday in a week from now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants