S219 : datastore databases

[eschultink]

Features

• support pipeline data being stored in non-default Firestore database(s)
• tests for pipeline data being stored in non-default Firestore database(s), namespace(s)

Change implications

• breaking change to API? no
• changes dependencies? no

[Copilot]

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.DatastoreNamespace and propagates these through JobRecord, PipelineModelObject key generation, and QueueSettings.
• Serializes/deserializes datastore boundary settings on PipelineTask via 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.

[Copilot]

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());
	}

[Copilot]

getSettingValue() uses s.getClass().isAssignableFrom(clazz), which is the reverse of the usual assignability check. This will fail to match if a setting instance is a subclass of clazz (or if clazz is an interface/supertype you want to match broadly). Use clazz.isAssignableFrom(s.getClass()) (or clazz.isInstance(s)) to correctly detect matching settings.

Suggested change

	.filter(s -> s.getClass().isAssignableFrom(clazz))
	.filter(clazz::isInstance)

[Copilot]

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).