palantir / conjure

Strongly typed HTTP/JSON APIs for browsers and microservices
https://palantir.github.io/conjure/
Apache License 2.0
417 stars 65 forks source link

Allow Unit type at least for Union Types #243

Open gm2211 opened 5 years ago

gm2211 commented 5 years ago

What happened?

BLUF: Some times the name of the union type in the union delivers enough information in itself and it should have type void (or Unit type).

NOTE: This is part of a pattern in which union types are used as enums but where some of the enum values should have additional info while others don't need to.

Example:

Gonna take a long-winded approach here Let's say I have a batch endpoint to get results for jobs - there are a few ways to model this:

// throws if insufficient permissions or any error for any
map<jobId, result> compute(...)

or

// return partial map if insufficient permissions or any error for any
map<jobId, result> compute(...)

or

// make result a union type that includes individual error states
map<jobId, result> compute(...)

Out of the 3 options, I would argue the latter gives us the most flexibility while at the same time forcing the client to make decisions about what to do for failures (while the partial map approach wouldn't).

This leads me to the actual issue at hand: how do we model the result union type to include error states. Here's one way

Result:
  union:
     insufficientPermissions: <WHAT_HERE>
     invalidRequest: SerializableError
     result: ResultWithMetrics

Now I can see 3 options that are currently available to replace <WHAT_HERE>:

  1. Create an object with extra info about the insufficient permissions error
  2. Use string and always pass an empty string
  3. Create "singleton" enum like this:
    InsufficientPermissions:
    values:
    - INSTANCE

Option 1. could work in this case, but there are some cases in which there really isn't much info to be provided at all. Option 2. is pretty confusing because if I see a string I expect it to contain something Option 3. is pretty verbose:

Result.insufficientPermissions(InsufficientPermissions.INSTANCE);

What did you want to happen?

I would like to be able to do this:

Result:
  union:
     insufficientPermissions: unit # or 'void'
     invalidRequest: SerializableError
     result: ResultWithMetrics

and

Result.insufficientPermissions();
berler commented 5 years ago

In internal projects, we currently create an Empty data type as a workaround. It is just slightly more annoying to work with (so definitely would prefer native support for this feature), but in the meantime you can do something like the following:

Empty:
  fields: {}
  docs: An empty marker type; can be used in unions to denote a variant with no data.

Then your union would look like:

Result:
  union:
    insufficientPermissions: Empty
    invalidRequest: SerializableError
    result: ResultWithMetrics

This way, constructing the empty union type looks like:

Result.insufficientPermissions(Empty.of());

I could even see the java generator being updated to recognize object types that have 0 fields, and create 0-arg union constructors (and visitors) for those types. Doing this would give some benefits at least in java while requiring no changes to the actual conjure yaml spec.

gm2211 commented 5 years ago

This is actually better: a zero field object allows the addition of fields without breaking the wire format. If conjure-java were to auto-generate a no-arg constructor we'd have the best of both worlds

Get Outlook for iOShttps://aka.ms/o0ukef


From: Steven Berler notifications@github.com Sent: Wednesday, March 6, 2019 5:10 PM To: palantir/conjure Cc: Giulio Mecocci; Author Subject: Re: [palantir/conjure] Allow Unit type at least for Union Types (#243)

In internal projects, we currently create an Empty data type as a workaround. It is just slightly more annoying to work with (so definitely would prefer native support for this feature), but in the meantime you can do something like the following:

Empty: fields: {} docs: An empty marker type; can be used in unions to denote a variant with no data.

Then your union would look like:

Result: union: insufficientPermissions: Empty invalidRequest: SerializableError result: ResultWithMetrics

This way, constructing the empty union type looks like:

Result.insufficientPermissions.of(Empty.of());

I could even see the java generator being updated to recognize object types that have 0 fields, and create 0-arg union constructors (and visitors) for those types. Doing this would give some benefits at least in java while requiring no changes to the actual conjure yaml spec.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/palantir/conjure/issues/243#issuecomment-470297739, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABe8BEaBupvsn9-CRA7xRxS09evOiVvOks5vUDzqgaJpZM4bYxDQ.