S219 : datastore databases - #94
Open
eschultink wants to merge 6 commits into
Open
eschultink wants to merge 6 commits into
eschultink wants to merge 6 commits into
Conversation
eschultink
requested review from
aperez-worklytics and
jlorper
and
a balanced review from Copilot
February 27, 2026 22:55
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for running App Engine Pipelines against non-default Cloud Datastore / Firestore-in-Datastore-mode database IDs and namespaces, and verifies propagation/serialization via new unit tests.
Changes:
- Introduces
JobSetting.DatastoreDatabase/JobSetting.DatastoreNamespaceand propagates these throughJobRecord,PipelineModelObjectkey generation, andQueueSettings. - Serializes/deserializes datastore boundary settings on
PipelineTaskvia task properties (dsDatabaseId,dsNamespace) and adds tests for round-tripping. - Adds Mockito MockMaker config and project agent/developer guidance.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| java/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker | Adds a Mockito mock maker configuration for tests. |
| java/src/test/java/com/google/appengine/tools/pipeline/impl/tasks/PipelineTaskTest.java | Tests task property round-trip including datastore database/namespace settings. |
| java/src/test/java/com/google/appengine/tools/pipeline/impl/model/PipelineModelObjectTest.java | Updates key-generation test to include databaseId parameter. |
| java/src/test/java/com/google/appengine/tools/pipeline/impl/model/JobRecordTest.java | Tests datastore database/namespace inheritance/override behavior in JobRecord. |
| java/src/test/java/com/google/appengine/tools/pipeline/JobSettingTest.java | Adds validation tests for new datastore database/namespace job settings. |
| java/src/main/java/com/google/appengine/tools/pipeline/impl/tasks/PipelineTask.java | Adds datastore database/namespace task properties and propagates them into task specs. |
| java/src/main/java/com/google/appengine/tools/pipeline/impl/model/PipelineModelObject.java | Updates key generation to include databaseId (and propagate from parent/root keys). |
| java/src/main/java/com/google/appengine/tools/pipeline/impl/model/JobRecord.java | Persists databaseId/namespace on job keys and inherits/overrides via JobSetting. |
| java/src/main/java/com/google/appengine/tools/pipeline/impl/QueueSettings.java | Adds databaseId and namespace settings and merges them during inheritance. |
| java/src/main/java/com/google/appengine/tools/pipeline/JobSetting.java | Adds DatastoreDatabase/DatastoreNamespace settings with validation. |
| AGENTS.md | Documents architecture and rules around datastore boundary propagation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+257
to
+264
| if (this.getQueueSettings().getDatabaseId() != null) { | ||
| spec.param("dsDatabaseId", this.getQueueSettings().getDatabaseId()); | ||
| } | ||
|
|
||
| if (this.getQueueSettings().getNamespace() != null) { | ||
| spec.param("dsNamespace", this.getQueueSettings().getNamespace()); | ||
| } | ||
|
|
There was a problem hiding this comment.
toTaskSpec() already adds all QueueSettings fields via this.toProperties().entrySet().forEach(...), and TaskProperty now includes dsDatabaseId/dsNamespace. The extra spec.param("dsDatabaseId"...) / spec.param("dsNamespace"...) block is redundant and can lead to empty-string values being forwarded as request params (since the guard is != null, not !isEmpty()), which then causes RequestUtils.buildDatastoreFromRequest() to call DatastoreOptions.Builder#setDatabaseId("") when the param is present. Consider removing the redundant block entirely, and/or normalizing databaseId/namespace so empty strings are treated as “unset” and not emitted as task params.
Suggested change
| if (this.getQueueSettings().getDatabaseId() != null) { | |
| spec.param("dsDatabaseId", this.getQueueSettings().getDatabaseId()); | |
| } | |
| if (this.getQueueSettings().getNamespace() != null) { | |
| spec.param("dsNamespace", this.getQueueSettings().getNamespace()); | |
| } |
Comment on lines
+42
to
+43
| * delay in seconds to set when enqueueing the task (eg, should not execute | ||
| * until *at least* this much time has passed |
There was a problem hiding this comment.
The Javadoc for delayInSeconds opens a parenthesis (“(eg, …”) but doesn’t close it, which reads like a truncated sentence. Consider closing the parenthesis or rephrasing into a full sentence for clarity.
Suggested change
| * delay in seconds to set when enqueueing the task (eg, should not execute | |
| * until *at least* this much time has passed | |
| * Delay in seconds to set when enqueueing the task (for example, the task should not execute | |
| * until at least this much time has passed). |
eschultink
marked this pull request as ready for review
March 9, 2026 20:30
…ing.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
aperez-worklytics
approved these changes
May 6, 2026
…del/PipelineModelObjectTest.java Co-authored-by: aperez-worklytics <75276364+aperez-worklytics@users.noreply.github.com>
…ing.java Co-authored-by: aperez-worklytics <75276364+aperez-worklytics@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The tests do not compile, and datastore selection and propagation currently prevent reliable non-default database execution.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (7)
Comment on lines
+492
to
+493
| databaseId = JobSetting.getSettingValue(JobSetting.DatastoreDatabase.class, settings) | ||
| .orElse(defaultDbId); |
Comment on lines
+524
to
+526
| String databaseId = JobSetting.getSettingValue(JobSetting.DatastoreDatabase.class, settings) | ||
| .orElse(null); | ||
| Key key = generateKey(projectId, databaseId, namespace, DATA_STORE_KIND); |
Comment on lines
+573
to
+575
| } else if (setting instanceof JobSetting.DatastoreNamespace || | ||
| setting instanceof JobSetting.DatastoreDatabase) { | ||
| // ignore; applied in constructor, bc they are final |
| assertEquals("project", key.getProjectId()); | ||
| assertEquals("ns", key.getNamespace()); | ||
| assertEquals("Kind", key.getKind()); | ||
| assertNull(key.getDatabase()) |
Comment on lines
+227
to
+228
| if (StringUtils.isNotBlank(datastoreDatabase) && !"default".equalsIgnoreCase(datastoreDatabase)) { | ||
| if (!datastoreDatabase.matches("^[a-z][a-z0-9-]{1,61}[a-z0-9]$")) { |
Comment on lines
+245
to
+247
| if (datastoreNameSpace != null) { | ||
| if (!datastoreNameSpace.matches("^[0-9A-Za-z._-]{0,100}$")) { | ||
| throw new IllegalArgumentException("Invalid Datastore namespace: " + datastoreNameSpace); |
Comment on lines
+50
to
+57
| @Nullable | ||
| private String databaseId; | ||
|
|
||
| /** | ||
| * datastore namespace to propagate | ||
| */ | ||
| @Nullable | ||
| private String namespace; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Features
Change implications