Skip to content
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

Allow a Callable to include a Genkit Action annotation #8039

Merged
merged 9 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
- Changes default CF3 runtime to nodejs22 (#8037)
- Fixed an issue where `--import` would error for the Data Connect emulator if `dataDir` was also set.
- Fixed an issue where `firebase init dataconnect` errored when importing a schema with no GQL files.
- CF3 callables can now be annotate with a genkit action they are serving (#8039)
- HTTPS functions can now be upgraded to HTTPS Callable functions (#8039)
- Update default tsconfig to support more modern defaults (#8039)
10 changes: 6 additions & 4 deletions src/deploy/functions/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { Runtime } from "./runtimes/supported";
import { FirebaseError } from "../../error";
import { Context } from "./args";
import { flattenArray } from "../../functional";
import { assertExhaustive, flattenArray } from "../../functional";

/** Retry settings for a ScheduleSpec. */
export interface ScheduleRetryConfig {
Expand Down Expand Up @@ -41,7 +41,9 @@
}

/** API agnostic version of a Firebase callable function. */
export type CallableTrigger = Record<string, never>;
export type CallableTrigger = {
genkitAction?: string;
};

/** Something that has a callable trigger */
export interface CallableTriggered {
Expand Down Expand Up @@ -135,6 +137,7 @@
eventType: string;
options?: Record<string, unknown>;
}

export interface BlockingTriggered {
blockingTrigger: BlockingTrigger;
}
Expand All @@ -153,9 +156,8 @@
return "taskQueue";
} else if (isBlockingTriggered(endpoint)) {
return endpoint.blockingTrigger.eventType;
} else {
throw new Error("Unexpected trigger type for endpoint " + JSON.stringify(endpoint));
}
assertExhaustive(endpoint);
}

// TODO(inlined): Enum types should be singularly named
Expand Down Expand Up @@ -514,7 +516,7 @@
await loadExistingBackend(context);
}
// loadExisting guarantees the validity of existingBackend and unreachableRegions
return context.existingBackend!;

Check warning on line 519 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
}

async function loadExistingBackend(ctx: Context): Promise<void> {
Expand Down Expand Up @@ -547,8 +549,8 @@
ctx.existingBackend.endpoints[endpoint.region][endpoint.id] = endpoint;
}
ctx.unreachableRegions.gcfV2 = gcfV2Results.unreachable;
} catch (err: any) {

Check warning on line 552 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
if (err.status === 404 && err.message?.toLowerCase().includes("method not found")) {

Check warning on line 553 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value

Check warning on line 553 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .includes on an `any` value

Check warning on line 553 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe call of an `any` typed value

Check warning on line 553 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .message on an `any` value

Check warning on line 553 in src/deploy/functions/backend.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe call of an `any` typed value
return; // customer has preview enabled without allowlist set
}
throw err;
Expand Down
9 changes: 6 additions & 3 deletions src/deploy/functions/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@

// Trigger definitions for RPCs servers using the HTTP protocol defined at
// https://firebase.google.com/docs/functions/callable-reference
// eslint-disable-next-line
interface CallableTrigger {}
interface CallableTrigger {
genkitAction?: string;
}

// Trigger definitions for endpoints that should be called as a delegate for other operations.
// For example, before user login.
Expand Down Expand Up @@ -319,7 +320,7 @@
}

// Exported for testing
export function envWithTypes(

Check warning on line 323 in src/deploy/functions/build.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Missing JSDoc comment
definedParams: params.Param[],
rawEnvs: Record<string, string>,
): Record<string, params.ParamValue> {
Expand Down Expand Up @@ -465,7 +466,7 @@
// List param, we try resolving a String param instead.
try {
regions = params.resolveList(bdEndpoint.region, paramValues);
} catch (err: any) {

Check warning on line 469 in src/deploy/functions/build.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
if (err instanceof ExprParseError) {
regions = [params.resolveString(bdEndpoint.region, paramValues)];
} else {
Expand Down Expand Up @@ -568,7 +569,9 @@
}
return { httpsTrigger };
} else if (isCallableTriggered(endpoint)) {
return { callableTrigger: {} };
const trigger: CallableTriggered = { callableTrigger: {} };
proto.copyIfPresent(trigger.callableTrigger, endpoint.callableTrigger, "genkitAction");
return trigger;
} else if (isBlockingTriggered(endpoint)) {
return { blockingTrigger: endpoint.blockingTrigger };
} else if (isEventTriggered(endpoint)) {
Expand Down
10 changes: 10 additions & 0 deletions src/deploy/functions/release/planner.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ describe("planner", () => {
expect(() => planner.calculateUpdate(httpsFunc, scheduleFunc)).to.throw();
});

it("allows upgrades of genkit functions from the genkit plugin to firebase-functions SDK", () => {
const httpsFunc = func("a", "b", { httpsTrigger: {} });
const genkitFunc = func("a", "b", { callableTrigger: { genkitAction: "flows/flow" } });
expect(planner.calculateUpdate(genkitFunc, httpsFunc)).to.deep.equal({
// Missing: deleteAndRecreate
endpoint: genkitFunc,
unsafe: false,
});
});

it("knows to delete & recreate for v2 topic changes", () => {
const original: backend.Endpoint = {
...func("a", "b", {
Expand Down
15 changes: 8 additions & 7 deletions src/deploy/functions/release/planner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@
import * as utils from "../../../utils";
import * as backend from "../backend";
import * as v2events from "../../../functions/events/v2";
import {
FIRESTORE_EVENT_REGEX,
FIRESTORE_EVENT_WITH_AUTH_CONTEXT_REGEX,
} from "../../../functions/events/v2";

export interface EndpointUpdate {
endpoint: backend.Endpoint;
Expand Down Expand Up @@ -232,7 +228,7 @@
if (have.eventTrigger.eventType !== v2events.PUBSUB_PUBLISH_EVENT) {
return false;
}
return have.eventTrigger.eventFilters!.topic !== want.eventTrigger.eventFilters!.topic;

Check warning on line 231 in src/deploy/functions/release/planner.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
}

/** Whether a user upgraded a scheduled function (which goes from Pub/Sub to HTTPS). */
Expand Down Expand Up @@ -261,9 +257,9 @@
export function checkForUnsafeUpdate(want: backend.Endpoint, have: backend.Endpoint): boolean {
return (
backend.isEventTriggered(want) &&
FIRESTORE_EVENT_WITH_AUTH_CONTEXT_REGEX.test(want.eventTrigger.eventType) &&
backend.isEventTriggered(have) &&
FIRESTORE_EVENT_REGEX.test(have.eventTrigger.eventType)
want.eventTrigger.eventType ===
v2events.CONVERTABLE_EVENTS[have.eventTrigger.eventType as v2events.Event]
);
}

Expand All @@ -289,7 +285,12 @@
};
const wantType = triggerType(want);
const haveType = triggerType(have);
if (wantType !== haveType) {

// Originally, @genkit-ai/firebase/functions defined onFlow which created an HTTPS trigger that implemented the streaming callable protocol for the Flow.
// The new version is firebase-functions/https which defines onCallFlow
const upgradingHttpsFunction =
backend.isHttpsTriggered(have) && backend.isCallableTriggered(want);
if (wantType !== haveType && !upgradingHttpsFunction) {
throw new FirebaseError(
`[${getFunctionLabel(
want,
Expand Down
29 changes: 29 additions & 0 deletions src/deploy/functions/runtimes/discovery/v1alpha1.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,35 @@ describe("buildFromV1Alpha", () => {
});
});

describe("genkitTriggers", () => {
it("fails with invalid fields", () => {
assertParserError({
endpoints: {
func: {
...MIN_ENDPOINT,
genkitTrigger: {
tool: "tools are not supported",
},
},
},
});
});

it("cannot be used with 1st gen", () => {
assertParserError({
endpoints: {
func: {
...MIN_ENDPOINT,
platform: "gcfv1",
genkitTrigger: {
flow: "agent",
},
},
},
});
});
});

describe("scheduleTriggers", () => {
const validTrigger: build.ScheduleTrigger = {
schedule: "every 5 minutes",
Expand Down
6 changes: 5 additions & 1 deletion src/deploy/functions/runtimes/discovery/v1alpha1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@ function assertBuildEndpoint(ep: WireEndpoint, id: string): void {
invoker: "array?",
});
} else if (build.isCallableTriggered(ep)) {
// no-op
assertKeyTypes(prefix + ".callableTrigger", ep.callableTrigger, {
genkitAction: "string?",
});
} else if (build.isScheduleTriggered(ep)) {
assertKeyTypes(prefix + ".scheduleTrigger", ep.scheduleTrigger, {
schedule: "Field<string>",
Expand Down Expand Up @@ -263,6 +265,7 @@ function assertBuildEndpoint(ep: WireEndpoint, id: string): void {
options: "object",
});
} else {
// TODO: Replace with assertExhaustive, which needs some type magic here because we have an any
throw new FirebaseError(
`Do not recognize trigger type for endpoint ${id}. Try upgrading ` +
"firebase-tools with npm install -g firebase-tools@latest",
Expand Down Expand Up @@ -310,6 +313,7 @@ function parseEndpointForBuild(
copyIfPresent(triggered.httpsTrigger, ep.httpsTrigger, "invoker");
} else if (build.isCallableTriggered(ep)) {
triggered = { callableTrigger: {} };
copyIfPresent(triggered.callableTrigger, ep.callableTrigger, "genkitAction");
} else if (build.isScheduleTriggered(ep)) {
const st: build.ScheduleTrigger = {
// TODO: consider adding validation for fields like this that reject
Expand Down
18 changes: 14 additions & 4 deletions src/functions/events/v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ export const FIRESTORE_EVENTS = [

export const FIREALERTS_EVENT = "google.firebase.firebasealerts.alerts.v1.published";

export const FIRESTORE_EVENT_REGEX = /^google\.cloud\.firestore\.document\.v1\.[^\.]*$/;
export const FIRESTORE_EVENT_WITH_AUTH_CONTEXT_REGEX =
/^google\.cloud\.firestore\.document\.v1\..*\.withAuthContext$/;

export type Event =
| typeof PUBSUB_PUBLISH_EVENT
| (typeof STORAGE_EVENTS)[number]
Expand All @@ -46,3 +42,17 @@ export type Event =
| typeof TEST_LAB_EVENT
| (typeof FIRESTORE_EVENTS)[number]
| typeof FIREALERTS_EVENT;

// Why can't auth context be removed? This is map was added to correct a bug where a regex
// allowed any non-auth type to be converted to any auth type, but we should follow up for why
// a functon can't opt into reducing PII.
export const CONVERTABLE_EVENTS: Partial<Record<Event, Event>> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this called "covertable" when we use this mapping to check whether event is not convertable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it holds the list of events that are convertable. Not convertable means not in the map

"google.cloud.firestore.document.v1.created":
"google.cloud.firestore.document.v1.created.withAuthContext",
"google.cloud.firestore.document.v1.updated":
"google.cloud.firestore.document.v1.updated.withAuthContext",
"google.cloud.firestore.document.v1.deleted":
"google.cloud.firestore.document.v1.deleted.withAuthContext",
"google.cloud.firestore.document.v1.written":
"google.cloud.firestore.document.v1.written.withAuthContext",
};
40 changes: 40 additions & 0 deletions src/gcp/cloudfunctionsv2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,23 @@ describe("cloudfunctionsv2", () => {
[BLOCKING_LABEL]: "before-sign-in",
},
});

expect(
cloudfunctionsv2.functionFromEndpoint({
...ENDPOINT,
platform: "gcfv2",
callableTrigger: {
genkitAction: "flows/flow",
},
}),
).to.deep.equal({
...CLOUD_FUNCTION_V2,
labels: {
...CLOUD_FUNCTION_V2.labels,
"deployment-callable": "true",
"genkit-action": "flows/flow",
},
});
});

it("should copy trival fields", () => {
Expand Down Expand Up @@ -637,6 +654,29 @@ describe("cloudfunctionsv2", () => {
});
});

it("should translate genkit callables", () => {
expect(
cloudfunctionsv2.endpointFromFunction({
...HAVE_CLOUD_FUNCTION_V2,
labels: {
"deployment-callable": "true",
"genkit-action": "flows/flow",
},
}),
).to.deep.equal({
...ENDPOINT,
callableTrigger: {
genkitAction: "flows/flow",
},
platform: "gcfv2",
uri: GCF_URL,
labels: {
"deployment-callable": "true",
"genkit-action": "flows/flow",
},
});
});

it("should copy optional fields", () => {
const extraFields: backend.ServiceConfiguration = {
ingressSettings: "ALLOW_ALL",
Expand Down
6 changes: 6 additions & 0 deletions src/gcp/cloudfunctionsv2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,9 @@ export function functionFromEndpoint(endpoint: backend.Endpoint): InputCloudFunc
gcfFunction.labels = { ...gcfFunction.labels, "deployment-taskqueue": "true" };
} else if (backend.isCallableTriggered(endpoint)) {
gcfFunction.labels = { ...gcfFunction.labels, "deployment-callable": "true" };
if (endpoint.callableTrigger.genkitAction) {
gcfFunction.labels["genkit-action"] = endpoint.callableTrigger.genkitAction;
}
} else if (backend.isBlockingTriggered(endpoint)) {
gcfFunction.labels = {
...gcfFunction.labels,
Expand Down Expand Up @@ -654,6 +657,9 @@ export function endpointFromFunction(gcfFunction: OutputCloudFunction): backend.
trigger = {
callableTrigger: {},
};
if (gcfFunction.labels["genkit-action"]) {
trigger.callableTrigger.genkitAction = gcfFunction.labels["genkit-action"];
}
} else if (gcfFunction.labels?.[BLOCKING_LABEL]) {
trigger = {
blockingTrigger: {
Expand Down
4 changes: 3 additions & 1 deletion templates/init/functions/typescript/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{
"compilerOptions": {
"module": "commonjs",
"module": "NodeNext",
"esModuleInterop": true,
"moduleResolution": "nodenext",
"noImplicitReturns": true,
"noUnusedLocals": true,
"outDir": "lib",
Expand Down
Loading