Skip to content

Allows non ancestor queries inside a transaction #366

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

Merged
merged 4 commits into from
May 5, 2025

Conversation

step76
Copy link
Contributor

@step76 step76 commented Apr 24, 2025

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
Copy link
Collaborator

ludoch commented Apr 25, 2025

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
Copy link
Contributor Author

step76 commented Apr 25, 2025

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
Copy link
Collaborator

maigovannon commented Apr 25, 2025

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
Copy link
Contributor Author

step76 commented Apr 25, 2025

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
Copy link
Collaborator

ludoch commented Apr 25, 2025

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

@step76
Copy link
Contributor Author

step76 commented Apr 26, 2025

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.

Copy link
Collaborator

@ludoch ludoch left a comment

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
Copy link
Collaborator

ludoch commented Apr 28, 2025

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
6 checks passed
@ludoch
Copy link
Collaborator

ludoch commented May 5, 2025

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