smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

Unwrap Operation output when its structure contains a single field

Open bpholt opened this issue 1 year ago • 6 comments

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

bpholt avatar Dec 14 '24 00:12 bpholt

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?

kubukoz avatar Dec 16 '24 14:12 kubukoz

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.

bpholt avatar Dec 16 '24 18:12 bpholt

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?

kubukoz avatar Dec 16 '24 19:12 kubukoz

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

Baccata avatar Jan 02 '25 09:01 Baccata

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

kubukoz avatar Jan 02 '25 15:01 kubukoz

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.

Baccata avatar Jan 02 '25 15:01 Baccata