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

Unwrap Operation output when its structure contains a single field #1627

Open
bpholt opened this issue Dec 14, 2024 · 6 comments
Open

Unwrap Operation output when its structure contains a single field #1627

bpholt opened this issue Dec 14, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@bpholt
Copy link
Contributor

bpholt commented Dec 14, 2024

I'm experimenting with using Smithy4s to define algebras using Smithy, and then generating all the typical stuff, to replace existing code that looks like this:

trait FooService[F[_]]
  def saveFoo(foo: Foo): F[Unit]
  def getFooById(id: Identifier): F[Option[Foo]]
}

Everything works pretty well, except that I haven't been able to figure out how to make getFooById return F[Option[Foo]]. I can have it emit F[Foo], or F[GetFooByIdOutput] with a case class GetFooByIdOutput(output: Option[Foo]), but I can't strip out the wrapper class without losing the optionality.

Am I correct in understanding that it is not currently possible to write a Smithy file that Smithy4s would generate output like I'm looking for?

service FooService {
  version: "1.0.0"
  operations: [GetFooById]
}
@packedInputs operation GetFooById {
  input: GetFooByIdInput
  output: GetFooByIdOutput
}
structure Foo {
  id: Identifier
}
structure GetFooByIdInput {
  @required id: Identifier
}
structure GetFooByIdOutput {
  output: Foo
}
string Identifier

(This would also come up if I wanted to return F[List[Foo]], I think, because I tried writing an unwrapped type refinement to turn List[Foo] into Option[Foo] and then have

operation GetFooById {
  input: GetFooByIdInput
  output: Foos
}
list Foos { … }

but that doesn't work, because operation shape output relationships must target a structure shape.)

@kubukoz
Copy link
Member

kubukoz commented Dec 16, 2024

Smithy4s doesn't support flattening output types, so you're right, this is currently not possible.

It could be added with a trait similar to packedInputs (unpackedOutput?), but I'm not sure if this is a direction we want to pursue - codegen customizations always involve risk that we end up with more complexity (due to how many combinations of generated code we have to test), so they need to carry their weight.

With packedInputs, it's pretty clear that when you have a lot of input parameters they become difficult to work with / pass forward / update, but the output unpacking would be a 1<->1 mapping, so I'm not convinced yet.

What's the usecase, in the bigger picture?

@bpholt
Copy link
Contributor Author

bpholt commented Dec 16, 2024

Ultimately, I think it's just an ergonomics thing. When I wrote up this issue I was focused on recreating a specific interface, so getting to F[Option[Foo]] was my goal, but on further reflection and investigation, I have found that it's very common for our operation return types to only contain a single field. Looking at our existing RPC data structures, about half contain only a single field which is either a primitive, list, or map.

Adding an extra .map(_.value) onto those Smithy4s method calls isn't a huge deal, but it's a little annoying and doesn't feel very idiomatic.

It could be added with a trait similar to packedInputs (unpackedOutput?)

When a response structure only contains a single field, I would suggest unwrapping it be the default, with the current behavior made optional using a packedOutputs trait, but an unpackedOutput trait would be a great option if you don't want to change the default behavior.

@kubukoz
Copy link
Member

kubukoz commented Dec 16, 2024

OK, I see your point of view now. Indeed, I'm concerned about changing default behavior, but let's see whether this whole thing is something we should do first - @Baccata thoughts?

@Baccata
Copy link
Contributor

Baccata commented Jan 2, 2025

Yeah we ain't gonna change the default behaviour, but we could certainly add an @unpackedOutput meta trait.

@kubukoz kubukoz added the enhancement New feature or request label Jan 2, 2025
@kubukoz
Copy link
Member

kubukoz commented Jan 2, 2025

@bpholt this is up for grabs, then - if you'd like to pick it up, let me know if you need assistance

@Baccata
Copy link
Contributor

Baccata commented Jan 2, 2025

NB : I've still not managed to get a corporate CLA, which @bpholt had asked for. I think this contribution would be substantial enough to warrant a signature, but at the same time it's a low-hanging-fruit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants