S219 : datastore databases#94
Conversation
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.
| 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.
| if (this.getQueueSettings().getDatabaseId() != null) { | |
| spec.param("dsDatabaseId", this.getQueueSettings().getDatabaseId()); | |
| } | |
| if (this.getQueueSettings().getNamespace() != null) { | |
| spec.param("dsNamespace", this.getQueueSettings().getNamespace()); | |
| } |
| * 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.
| * 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). |
…ing.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| public DatastoreDatabase(String datastoreDatabase) { | ||
| super(datastoreDatabase); | ||
| if (datastoreDatabase != null && !datastoreDatabase.isEmpty() && !datastoreDatabase.equals("(default)")) { |
There was a problem hiding this comment.
| if (datastoreDatabase != null && !datastoreDatabase.isEmpty() && !datastoreDatabase.equals("(default)")) { | |
| if (StringUtils.isNotBlank(datastoreDatabase) && !"default".equalsIgnoreCase(datastoreDatabase)) { |
|
|
||
| assertEquals("project", key.getProjectId()); | ||
| assertEquals("ns", key.getNamespace()); | ||
| assertEquals("Kind", key.getKind()); |
There was a problem hiding this comment.
| assertEquals("Kind", key.getKind()); | |
| assertEquals("Kind", key.getKind()); | |
| assertNull(key.getDatabase()) |
| Key key = generateKey(projectId, namespace, DATA_STORE_KIND); | ||
| .orElse(null); | ||
| String databaseId = JobSetting.getSettingValue(JobSetting.DatastoreDatabase.class, settings) | ||
| .orElse(null); |
There was a problem hiding this comment.
So when null it will use default one ?
Features
Change implications