zio / zio-aws

Low level ZIO interface for the full AWS
https://zio.dev/zio-aws
Apache License 2.0
139 stars 33 forks source link

Question: Could we statically represent credentials provider using ADTs such that configuration library can report/document them back? #144

Open afsalthaj opened 4 years ago

vigoo commented 4 years ago

I could definitely create more ADT wrappers for some AWS types such as the credentials provider but I don't understand how this would help.

Currently we have this:

val credentialsProvider: ConfigDescriptor[AwsCredentialsProvider] = {
      val defaultCredentialsProvider: ConfigDescriptor[AwsCredentialsProvider] =
        string.xmapEither(
          s =>
            if (s == "default") Right(DefaultCredentialsProvider.create())
            else Left("Not 'default'"),
          {
            case _: DefaultCredentialsProvider => Right("default")
            case _                             => Left("Unsupported credentials provider")
          }
        )
      val anonymousCredentialsProvider
          : ConfigDescriptor[AwsCredentialsProvider] = string.xmapEither(
        s =>
          if (s == "anonymous") Right(AnonymousCredentialsProvider.create())
          else Left("Not 'anonymous'"),
        {
          case _: AnonymousCredentialsProvider => Right("anonymous")
          case _                               => Left("Unsupported credentials provider")
        }
      )
      val staticCredentialsProvider: ConfigDescriptor[AwsCredentialsProvider] =
        awsCredentials.xmap(
          creds => StaticCredentialsProvider.create(creds),
          _.resolveCredentials()
        )

      defaultCredentialsProvider <> anonymousCredentialsProvider <> staticCredentialsProvider
    }

If it would not use the underlying credentials provider directly then the only difference would be that instead of string("x").xmapEither(...) we could write it in a more concise way as string("x")(X.apply, X.unapply) but that gives the same underlying XmapEither wrapped descriptor. Your new enumeration helper in https://github.com/zio/zio-config/pull/451 also does this. So maybe there is something I don't see here :)

On the other hand there are some descriptors marked with NOTE: Cannot extract conditions without reflection comments which does not have a way to convert back to config value, for those it would definitely make sense to create a wrapper.

afsalthaj commented 4 years ago

Static representation using ADTs would document the constants in your markdown even if there is Xmap node involved in it. Having an Xmap doesn't necessarily mean you lose the informations because it's all going to be RunTime.

Example of such a markdown for a case class that uses ADTs is given below:


sealed trait Auth

@name("default")
case object Default extends Auth

@name("credentials")
case class Credentials(username: String, password: String) extends Auth

case class MyConfig(auth: Auth)

Markdown will say:

fieldName. format. description
auth all-of[Auth]

Auth

fieldName format description.
credentials all-of [Credentials]
Constant value "default" value of type string

Credentials

fieldName format description
username string value of type string
password string value of type string

On top of this, if you want to generate a sample config using zio-config-gen , pushing more things towards static boundaries would help. For example, consider the case where you end up writing the below write function in xmap.

string("auth")
  .xmapEither(
    s => if (s == "default") Right(Default) else Left("Supports only default"), 
    case Default => Right("default")
  )

(yes, zio-config-magnolia would be a better candidate here to avoid the question of what if I do case Default => "bla")

The above code would mean that when writing a randomly generated config ensure that value is going to be default, and not anything random.

Long story short, generally prefer pushing it to ADTs whenever there is a coproductish use case.

afsalthaj commented 4 years ago

I agree that in the markdown, not having name for that constant value Default can be sort of making us feel mmm. However its a view thing. Point is we are grabbing that info. Also table is exposed for any other view. We can make it work with Json as well in near future.

Along with it, the upcoming zio-config-gen (PR raised) would allow the user to edit a randomly generated (yet mostly correct) config. And the correctness here would be a function of how much we represent things statically.

afsalthaj commented 4 years ago

If you are happy, I can sort of work on the config side of things. A few changes are already in my fork. Give me this week time though :)

vigoo commented 4 years ago

Thanks for the detailed explanation. I'm not definitely against introducing more wrappers here but my point is that the hand-written descriptors should be equally powerful than the derived ones.

Now I checked this in zio-config to understand better your point and realized that this is not the case. There is a ConstantString PropertyType in DerivationUtils and so that's a special case only used by derivation. I would expose constant beside the other config descriptors to all users and then it can be used in my hand-written descriptors too :) I will open a zio-config issue about this so we can discuss there if you agree or not.

I also checked the config generator PR and looked to me that it does not contain the part yet that would handle this and I have some similar to above concerns related to it, I'll write a comment soon about it there.

afsalthaj commented 4 years ago

Sure. We can discuss and it’s worth it if it’s for the improvement of zio ecosystem.

As of now all I can say is even in hand written one, ADTS is going to help in generated random config/documentations.

https://github.com/zio/zio-config/blob/gen/examples/src/main/scala/zio/config/examples/RandomConfigGenerationComplexExample.scala

Agree that the semantics of constants can some how be part of manual derivations. We need a sub division of description that can hold the information of “type”, “default” etc. It’s already in the vision. But i still imagine it’s not a deal breaker. magnolia knows ur static type and hence it produces that doc and in fact possess a bit more power with its meta programming capability.

However, it’s going to be a deal breaker if there is no static representation of a config. For example, if AwsCredentialsprovider comes into xmapEither, then yes we are stuck coz that is not a config yet but something a bit beyond. My changes in fork put forward an actual intermediate representation of the config. Summoning actual AWS clients, connections etc is dealt when it’s required snd acquired after potentially a pattern match on types under ZIO effects (and not Either of xmapEither).

This is a good discussion. We can work together.

On Sun, 29 Nov 2020 at 11:48 PM, Daniel Vigovszky notifications@github.com wrote:

Thanks for the detailed explanation. I'm not definitely against introducing more wrappers here but my point is that the hand-written descriptors should be equally powerful than the derived ones.

Now I checked this in zio-config to understand better your point and realized that this is not the case. There is a ConstantString PropertyType in DerivationUtils and so that's a special case only used by derivation. I would expose constant beside the other config descriptors to all users and then it can be used in my hand-written descriptors too :) I will open a zio-config issue about this so we can discuss there if you agree or not.

I also checked the config generator PR and looked to me that it does not contain the part yet that would handle this and I have some similar to above concerns related to it, I'll write a comment soon about it there.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/vigoo/zio-aws/issues/144#issuecomment-735388594, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABY2QJI3QEDX3CJJZNGCML3SSI7I7ANCNFSM4UCM3QXA .