diff --git a/GUIDE.md b/GUIDE.md index 653238de8..c81a18aab 100644 --- a/GUIDE.md +++ b/GUIDE.md @@ -568,6 +568,14 @@ Key observations: 2. `_update_email(request)` -- internal standalone, takes request object 3. `update_email(*, user_id, email)` -- public standalone convenience +In TypeScript, a resource holds a 64-bit integer as a `Long`. A record without +`@nexus.proto` holds it as a `number`, so nexgen refuses a resource field or +method parameter of that type that goes into such a request record, also through +a proto message inside it. It also refuses a resource with such a field that an +operation returns with no proto wire message. To fix it, bind the record or the +result type to a proto message, or use another type. nexgen also refuses a +64-bit map key in any TypeScript resource, since a `Long` can't key a `Record`. + ### Name-Binding: How Resource Methods Find Operations This is the non-obvious part. The generator does not require you to explicitly @@ -823,6 +831,9 @@ enum's name and members, even when its name differs from the protobuf enum's. Converters bridge the model and protobuf enum by number, and `@nexus.default` continues to name a WIT case. +In TypeScript, a 64-bit integer is a `Long` in a proto-backed model, and a +`number` in other models. + ### Transfer-Type Conversion ```wit diff --git a/advanced/samples/README.md b/advanced/samples/README.md index cc1f7e872..0283ada51 100644 --- a/advanced/samples/README.md +++ b/advanced/samples/README.md @@ -165,9 +165,7 @@ model. nexgen ignores the directives that it doesn't know yet, such as Python imports the Stream protos from `temporalio.api`, where a `temporalio` release with the protos will have them. Until then, the sample installs them -there from its own `temporalio-stream-protos` package. TypeScript passes a -`number` where a resource method's proto request wants a `Long`. So `tsc` skips -the service, but the test still checks its models. +there from its own `temporalio-stream-protos` package. - WIT: [`inputs/stream-service.wit`](inputs/stream-service.wit) - .NET: [`dotnet/wit/stream-service/`](dotnet/wit/stream-service/), diff --git a/advanced/samples/invalid/resources/typescript-64-bit-map-key/expected.toml b/advanced/samples/invalid/resources/typescript-64-bit-map-key/expected.toml new file mode 100644 index 000000000..698a32b6c --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-map-key/expected.toml @@ -0,0 +1,8 @@ +languages = ["typescript"] + +[error] +kind = "invalid-resource-method" +contains = [ + "CounterService.Counter.add", + "parameter `totals` has a 64-bit map key", +] diff --git a/advanced/samples/invalid/resources/typescript-64-bit-map-key/input.wit b/advanced/samples/invalid/resources/typescript-64-bit-map-key/input.wit new file mode 100644 index 000000000..52a89d7f7 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-map-key/input.wit @@ -0,0 +1,12 @@ +package temporal:counters@1.0.0; + +world system { export counter-service; } + +/// @nexus.endpoint "__counter_service" +interface counter-service { + resource counter { + constructor(counter-id: string); + + add: func(totals: map); + } +} diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/expected.toml b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/expected.toml new file mode 100644 index 000000000..5a2fec736 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/expected.toml @@ -0,0 +1,8 @@ +languages = ["typescript"] + +[error] +kind = "invalid-resource-method" +contains = [ + "CounterService.Counter.add", + "field `total` goes into record `counter-service.add-request`", +] diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/input.wit b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/input.wit new file mode 100644 index 000000000..7dbbd730b --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-field/input.wit @@ -0,0 +1,19 @@ +package temporal:counters@1.0.0; + +world system { export counter-service; } + +/// @nexus.endpoint "__counter_service" +interface counter-service { + resource counter { + constructor(counter-id: string, total: s64); + + add: func(); + } + + record add-request { + counter-id: string, + total: s64, + } + + add: func(request: add-request); +} diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/expected.toml b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/expected.toml new file mode 100644 index 000000000..5769972f4 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/expected.toml @@ -0,0 +1,9 @@ +languages = ["typescript"] +descriptors = "temporal" + +[error] +kind = "invalid-resource-method" +contains = [ + "CounterService.Counter.touch", + "parameter `seconds` goes into record `counter-service.touch-request`", +] diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/input.wit b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/input.wit new file mode 100644 index 000000000..656bbaace --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request-proto-member/input.wit @@ -0,0 +1,25 @@ +package temporal:counters@1.0.0; + +world system { export counter-service; } + +/// @nexus.endpoint "__counter_service" +interface counter-service { + /// @nexus.proto "google.protobuf.Timestamp" + record timestamp { + seconds: option, + nanos: option, + } + + resource counter { + constructor(counter-id: string); + + touch: func(seconds: option); + } + + record touch-request { + counter-id: string, + at: option, + } + + touch: func(request: touch-request); +} diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request/expected.toml b/advanced/samples/invalid/resources/typescript-64-bit-plain-request/expected.toml new file mode 100644 index 000000000..ee6149d80 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request/expected.toml @@ -0,0 +1,10 @@ +languages = ["typescript"] + +[error] +kind = "invalid-resource-method" +contains = [ + "CounterService.Counter.add", + "parameter `amount` goes into record `counter-service.add-request`", + "has no `@nexus.proto`", + "A TypeScript resource holds it as a `Long`", +] diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-request/input.wit b/advanced/samples/invalid/resources/typescript-64-bit-plain-request/input.wit new file mode 100644 index 000000000..c60c98638 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-request/input.wit @@ -0,0 +1,19 @@ +package temporal:counters@1.0.0; + +world system { export counter-service; } + +/// @nexus.endpoint "__counter_service" +interface counter-service { + resource counter { + constructor(counter-id: string); + + add: func(amount: s64); + } + + record add-request { + counter-id: string, + amount: s64, + } + + add: func(request: add-request); +} diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-result/expected.toml b/advanced/samples/invalid/resources/typescript-64-bit-plain-result/expected.toml new file mode 100644 index 000000000..ad5466e97 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-result/expected.toml @@ -0,0 +1,9 @@ +languages = ["typescript"] + +[error] +kind = "invalid-resource" +contains = [ + "CounterService.Counter", + "operation `GetCounter` returns it with no proto wire message", + "64-bit field `total` goes over JSON as a `number`", +] diff --git a/advanced/samples/invalid/resources/typescript-64-bit-plain-result/input.wit b/advanced/samples/invalid/resources/typescript-64-bit-plain-result/input.wit new file mode 100644 index 000000000..588950821 --- /dev/null +++ b/advanced/samples/invalid/resources/typescript-64-bit-plain-result/input.wit @@ -0,0 +1,18 @@ +package temporal:counters@1.0.0; + +world system { export counter-service; } + +/// @nexus.endpoint "__counter_service" +interface counter-service { + resource counter { + constructor(counter-id: string, total: s64); + } + + type counter-result = own; + + record get-counter-request { + counter-id: string, + } + + get-counter: func(request: get-counter-request) -> counter-result; +} diff --git a/advanced/samples/typescript/tests/stream-service.test.ts b/advanced/samples/typescript/tests/stream-service.test.ts index 680ca8050..6dbe36210 100644 --- a/advanced/samples/typescript/tests/stream-service.test.ts +++ b/advanced/samples/typescript/tests/stream-service.test.ts @@ -1,7 +1,7 @@ import * as common from "@temporalio/common"; import type { temporal } from "@temporalio/proto"; import Long from "long"; -import { describe, expect, test } from "vitest"; +import { describe, expect, test, vi } from "vitest"; import { type ReadRecordsResponse, @@ -9,6 +9,12 @@ import { readRecordsRequestTransferTypeConverter, readRecordsResponseTransferTypeConverter, } from "../wit/stream-service/models.ts"; +import { Stream } from "../wit/stream-service/resources.ts"; + +const startOperation = vi.hoisted(() => vi.fn()); +vi.mock("@temporalio/workflow", () => ({ + createNexusServiceClient: () => ({ startOperation }), +})); describe("stream-service generated output", () => { // No SDK has these protos yet, so proto/streams.d.ts declares them. The converters fail to @@ -86,4 +92,24 @@ describe("stream-service generated output", () => { readRecordsRequestTransferTypeConverter.toTransferType(model).stream?.id, ).toBe("stream-1"); }); + + test("passes 64-bit values to a resource method's request as they are", async () => { + // An offset above 2^53 loses its low digits as a `number`. + const offset = Long.fromString("1880158843699200005"); + startOperation.mockResolvedValue({ + result: async () => ({ records: [], nextOffset: offset.add(1) }), + }); + const stream = new Stream({ id: "stream-1" }, undefined); + + const page = await stream.read(offset); + await stream.read(page.nextOffset); + + expect(startOperation.mock.calls.map(([, request]) => request.fromOffset)).toEqual([ + offset, + offset.add(1), + ]); + expect(String(startOperation.mock.calls[1][1].fromOffset)).toBe( + "1880158843699200006", + ); + }); }); diff --git a/advanced/samples/typescript/tsconfig.json b/advanced/samples/typescript/tsconfig.json index fe20217fc..44b6d192e 100644 --- a/advanced/samples/typescript/tsconfig.json +++ b/advanced/samples/typescript/tsconfig.json @@ -16,6 +16,5 @@ "support/**/*.ts", "tests/**/*.ts", "proto/**/*.d.ts" - ], - "exclude": ["wit/stream-service/**"] + ] } diff --git a/advanced/samples/typescript/wit/stream-service/resources.ts b/advanced/samples/typescript/wit/stream-service/resources.ts index 5dcb834e0..190522629 100644 --- a/advanced/samples/typescript/wit/stream-service/resources.ts +++ b/advanced/samples/typescript/wit/stream-service/resources.ts @@ -1,6 +1,7 @@ // Generated by nexgen latest. DO NOT EDIT! import type * as common from "@temporalio/common"; +import type Long from "long"; import type { StreamReadStart, StreamRef, @@ -33,12 +34,12 @@ export class Stream { } public async read( - fromOffset?: number, + fromOffset?: Long, start?: StreamReadStart, maxRecords?: number, wait?: common.Duration, createWait?: common.Duration, - maxBytes?: number, + maxBytes?: Long, ): Promise { const request = { stream: this.stream, diff --git a/changelog/fixed/long-lynx.md b/changelog/fixed/long-lynx.md new file mode 100644 index 000000000..147fd3208 --- /dev/null +++ b/changelog/fixed/long-lynx.md @@ -0,0 +1 @@ +A TypeScript resource holds a 64-bit integer as a `Long`, which keeps every digit of a Stream offset. nexgen refuses such a value where it would go into a record without `@nexus.proto`, which holds it as a `number`, and a 64-bit map key in any TypeScript resource, since a `Long` can't key a `Record`. diff --git a/src/generator/dotnet.rs b/src/generator/dotnet.rs index 6ee85e190..47ff04660 100644 --- a/src/generator/dotnet.rs +++ b/src/generator/dotnet.rs @@ -3709,6 +3709,7 @@ fn resource_method_request_model_init_expr( let RequestPlan::Construct { message_name, fields, + .. } = request_plan else { return None; @@ -4263,9 +4264,11 @@ fn with_resource_property_names(plan: &RequestPlan, type_name: &str) -> RequestP } RequestPlan::Construct { message_name, + proto, fields, } => RequestPlan::Construct { message_name: message_name.clone(), + proto: *proto, fields: fields .iter() .map(|field| RequestPlanField { diff --git a/src/generator/resource_plan.rs b/src/generator/resource_plan.rs index 7d7a8ffba..6b1b3cc75 100644 --- a/src/generator/resource_plan.rs +++ b/src/generator/resource_plan.rs @@ -27,6 +27,7 @@ where RequestPlan::Construct { message_name, fields, + .. } => { let rendered_fields = fields .iter() diff --git a/src/generator/typescript.rs b/src/generator/typescript.rs index 9c861c160..ac430aaeb 100644 --- a/src/generator/typescript.rs +++ b/src/generator/typescript.rs @@ -5,7 +5,7 @@ use std::path::{Path, PathBuf}; use heck::{ToKebabCase, ToLowerCamelCase, ToShoutySnakeCase, ToUpperCamelCase}; use indexmap::IndexMap; -use crate::error::Result; +use crate::error::{Error, Result}; use crate::generator::json_schema::typescript as typescript_json; use crate::generator::proto::typescript as typescript_proto; use crate::generator::proto::typescript::{ @@ -377,10 +377,45 @@ impl<'a> ApiPlanner<'a> { } } + // An operation that returns a resource with no proto wire message sends the resource as JSON. + fn check_plain_resource_output(&self, operation: &PlannedOperation) -> Result<()> { + if !operation.output_direct_result() { + return Ok(()); + } + let Some(PlannedType::Resource(output)) = operation.output_type() else { + return Ok(()); + }; + for service in &self.api_plan.services { + for resource in service.resources.iter().map(|resource| &resource.data) { + if resource.type_name != output.type_name { + continue; + } + let field = resource.fields.iter().find(|field| { + field.function.is_none() && typescript_holds_64_bit_int(&field.kind) + }); + if let Some(field) = field { + return Err(Error::InvalidResource { + service: service.name.clone(), + resource: resource.type_name.clone(), + reason: format!( + "operation `{}` returns it with no proto wire message, so its 64-bit \ + field `{}` goes over JSON as a `number`. {TYPESCRIPT_RESOURCE_LONG} \ + Bind the result type to a proto message with `@nexus.proto`, or use \ + another type", + operation.name, field.name + ), + }); + } + } + } + Ok(()) + } + fn resolve_operation<'operation>( &mut self, operation: &'operation PlannedOperation, ) -> Result> { + self.check_plain_resource_output(operation)?; let input = operation_input_model(operation); let output_transform = operation.output_transform.as_ref(); let output_resource_return = operation.data.output_resource_return.clone(); @@ -2150,6 +2185,8 @@ fn generate_leaf( for service in &services { for resource in &service.resources { + check_typescript_resource_map_keys(service.name, resource)?; + check_typescript_resource_json_requests(service.name, resource)?; planner.ensure_resource_field_types(resource); } } @@ -5844,8 +5881,10 @@ fn typescript_resource_field_annotation( } } +// A resource holds a 64-bit integer as a `Long`, since a number can't hold every value. fn typescript_resource_value_annotation(value: &PlannedType) -> String { match value { + PlannedType::Int(IntSpec::I64 | IntSpec::U64) => "Long".to_string(), PlannedType::Int(_) => "number".to_string(), PlannedType::Float => "number".to_string(), PlannedType::Bool => "boolean".to_string(), @@ -5918,9 +5957,164 @@ fn typescript_resource_field_requirements( } let mut requirements = TypeScriptRequirements::default(); collect_typescript_value_requirements(kind, &mut requirements); + requirements.long |= typescript_holds_64_bit_int(kind); requirements } +fn typescript_holds_64_bit_int(kind: &PlannedType) -> bool { + match kind { + PlannedType::Int(IntSpec::I64 | IntSpec::U64) => true, + PlannedType::Option(inner) | PlannedType::List(inner) => typescript_holds_64_bit_int(inner), + PlannedType::Map(key, value) => { + typescript_holds_64_bit_int(key) || typescript_holds_64_bit_int(value) + } + PlannedType::Tuple(items) => items.iter().any(typescript_holds_64_bit_int), + PlannedType::Result { ok, err } => { + ok.as_deref().is_some_and(typescript_holds_64_bit_int) + || err.as_deref().is_some_and(typescript_holds_64_bit_int) + } + PlannedType::External(ExternalTypeSpec::Alias(AliasTypeSpec { + type_name, + target: fallback, + .. + })) => type_name.value().is_none() && typescript_holds_64_bit_int(fallback), + _ => false, + } +} + +fn typescript_has_64_bit_map_key(kind: &PlannedType) -> bool { + match kind { + PlannedType::Map(key, value) => { + typescript_holds_64_bit_int(key) || typescript_has_64_bit_map_key(value) + } + PlannedType::Option(inner) | PlannedType::List(inner) => { + typescript_has_64_bit_map_key(inner) + } + PlannedType::Tuple(items) => items.iter().any(typescript_has_64_bit_map_key), + PlannedType::Result { ok, err } => { + ok.as_deref().is_some_and(typescript_has_64_bit_map_key) + || err.as_deref().is_some_and(typescript_has_64_bit_map_key) + } + PlannedType::External(ExternalTypeSpec::Alias(AliasTypeSpec { + type_name, + target: fallback, + .. + })) => type_name.value().is_none() && typescript_has_64_bit_map_key(fallback), + _ => false, + } +} + +const TYPESCRIPT_RESOURCE_LONG: &str = "A TypeScript resource holds it as a `Long`."; + +// A `Long` can't key a `Record`. +fn check_typescript_resource_map_keys(service: &str, resource: &PlannedResource) -> Result<()> { + let reason = |what: String| { + format!("{what} has a 64-bit map key, which a TypeScript resource would hold as a `Long`") + }; + for field in resource + .fields + .iter() + .filter(|field| field.function.is_none()) + { + if typescript_has_64_bit_map_key(&field.kind) { + return Err(Error::InvalidResource { + service: service.to_string(), + resource: resource.type_name.clone(), + reason: reason(format!("field `{}`", field.name)), + }); + } + } + for method in &resource.methods { + let result = method + .result + .as_ref() + .and_then(|result| match &result.kind { + PlannedResourceMethodResultKind::Value(kind) => Some(kind), + PlannedResourceMethodResultKind::Resource { .. } => None, + }); + let params = method + .params + .iter() + .filter(|param| param.function.is_none()) + .map(|param| (format!("parameter `{}`", param.name), ¶m.kind)); + for (what, kind) in params.chain(result.map(|kind| ("the result".to_string(), kind))) { + if typescript_has_64_bit_map_key(kind) { + return Err(Error::InvalidResourceMethod { + service: service.to_string(), + resource: resource.type_name.clone(), + method: method.name.clone(), + reason: reason(what), + }); + } + } + } + Ok(()) +} + +fn check_typescript_resource_json_requests( + service: &str, + resource: &PlannedResource, +) -> Result<()> { + for method in &resource.methods { + let PlannedResourceMethodBindingSpec::Operation { request_plan, .. } = &method.binding + else { + continue; + }; + if let Some((what, record)) = + typescript_plain_request_long(request_plan, resource, method, None) + { + return Err(Error::InvalidResourceMethod { + service: service.to_string(), + resource: resource.type_name.clone(), + method: method.name.clone(), + reason: format!( + "{what} goes into record `{record}`, which has no `@nexus.proto` and holds a \ + 64-bit integer as a `number`. {TYPESCRIPT_RESOURCE_LONG} Bind the record to \ + a proto message, or use another type" + ), + }); + } + } + Ok(()) +} + +/// The resource field or method parameter with a 64-bit integer that goes into a record with no +/// `@nexus.proto`, also through a proto message inside it, and that record. +fn typescript_plain_request_long<'a>( + plan: &'a RequestPlan, + resource: &PlannedResource, + method: &PlannedResourceMethod, + plain_record: Option<&'a str>, +) -> Option<(String, &'a str)> { + match plan { + RequestPlan::Source(source) => { + let record = plain_record?; + let (what, value) = match source { + RequestPlanSource::ResourceField(name) => ( + "field", + resource.fields.iter().find(|field| &field.name == name)?, + ), + RequestPlanSource::MethodParam(name) => ( + "parameter", + method.params.iter().find(|param| ¶m.name == name)?, + ), + }; + (value.function.is_none() && typescript_holds_64_bit_int(&value.kind)) + .then(|| (format!("{what} `{}`", value.name), record)) + } + RequestPlan::Construct { + message_name, + proto, + fields, + } => { + let plain_record = plain_record.or((!proto).then_some(message_name.as_str())); + fields.iter().find_map(|field| { + typescript_plain_request_long(&field.value, resource, method, plain_record) + }) + } + } +} + fn collect_typescript_value_requirements( value: &PlannedType, requirements: &mut TypeScriptRequirements, diff --git a/src/planning/resource_resolution.rs b/src/planning/resource_resolution.rs index 4cd468180..d34ee7393 100644 --- a/src/planning/resource_resolution.rs +++ b/src/planning/resource_resolution.rs @@ -158,6 +158,8 @@ pub enum RequestPlan { Source(RequestPlanSource), Construct { message_name: String, + /// Whether `message_name` is a proto message, not a WIT record. + proto: bool, fields: Vec, }, } @@ -547,6 +549,7 @@ fn synthesize_request_plan( Ok(Some(RequestPlan::Construct { message_name: message_name.to_string(), + proto: true, fields, })) } @@ -599,6 +602,7 @@ fn synthesize_record_request_plan( Ok(Some(RequestPlan::Construct { message_name: record_name.to_string(), + proto: false, fields, })) } @@ -1109,6 +1113,7 @@ interface user-service { let RequestPlan::Construct { message_name, fields, + .. } = request_plan else { panic!("request plan should construct update-email-request");