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

Allowing catch to work with any request types #2213

Closed
matt2e opened this issue Jul 31, 2024 · 2 comments · Fixed by #2398
Closed

Allowing catch to work with any request types #2213

matt2e opened this issue Jul 31, 2024 · 2 comments · Fixed by #2398
Assignees

Comments

@matt2e
Copy link
Collaborator

matt2e commented Jul 31, 2024

Currently when declaring a retry with a catch, the catch verb must include the exact request type. eg:

//ftl:verb
//ftl:retry 1 1s catch catchExample
func Example(ctx context.Context, req ExampleRequest) error {...}

//ftl:verb
func CatchExample(ctx context.Context, req builtin.CatchRequest[ExampleRequest]) error {...}

This prevents these use cases:

  • Defining a single catch verb that can be used for multiple retryable verbs (assuming they don't have the exact some request type)
  • Defining a catch in FSM's retry directive, as each transition has different event types

It could be useful to have a catch verb with takes builtin.CatchRequest[map[string]Any] or just builtin.CatchRequest[Any].

Something to be careful about:

  • A failing catch verb currently retries forever
  • If a request can't be unmarshalled into the request type at runtime we could easily end up repeatedly failing forever...
@github-actions github-actions bot added the triage Issue needs triaging label Jul 31, 2024
@ftl-robot ftl-robot mentioned this issue Jul 31, 2024
@alecthomas
Copy link
Collaborator

I think allowing Any makes sense.

@matt2e
Copy link
Collaborator Author

matt2e commented Jul 31, 2024

decision: Allow Any or exact match, add request type ref

@github-actions github-actions bot removed the triage Issue needs triaging label Jul 31, 2024
@matt2e matt2e changed the title Consider allowing catch to work with non-exact request types Allowing catch to work with any request types Aug 6, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 19, 2024
closes #2213

`builtin.CatchRequest` now has the following features:
- the associated request type can be `any` to allow for catching
multiple verbs with different request types
- The request will be unmarshalled to primitives (like string, int, map,
array etc) rather than ftl structs
- Includes the verb that failed as a `builtin.Ref`, which is a data type
with `module` and `name` strings
- Includes the request type as a string, eg: `example.Input`, `String`,
etc
- Originally I had this as a `builtin.Ref` but that doesnt work for
non-ref types

There is certainly room for improvement in terms of how we want to
expose these schema definitions, but this PR doesn't try go too far down
that road.
safeer pushed a commit that referenced this issue Aug 19, 2024
closes #2213

`builtin.CatchRequest` now has the following features:
- the associated request type can be `any` to allow for catching
multiple verbs with different request types
- The request will be unmarshalled to primitives (like string, int, map,
array etc) rather than ftl structs
- Includes the verb that failed as a `builtin.Ref`, which is a data type
with `module` and `name` strings
- Includes the request type as a string, eg: `example.Input`, `String`,
etc
- Originally I had this as a `builtin.Ref` but that doesnt work for
non-ref types

There is certainly room for improvement in terms of how we want to
expose these schema definitions, but this PR doesn't try go too far down
that road.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants