An alternate approach to enums #930
Conversation
| // enums defined in scala for which we want to validate the postgres enums and add to the schema | ||
| val scalaEnums: List[ScalaEnum] = | ||
| List( | ||
| ScalaEnum("e_too_activation", Enumerated[ToOActivation].toEnumType("ToOActivation", "Target of Opportunity Activation")(_.label)) |
There was a problem hiding this comment.
The description string ("Target of Opportunity Activiation") doesn't seem to be actually be used for anything? It would be nice if it could be the comment for the enum itself in the schema.
There was a problem hiding this comment.
However, we can add comments before the DUMMY entries in OdbSchema.graphql and they will be retained in the final schema.
There was a problem hiding this comment.
What happens if the validation with postgres fails?
There was a problem hiding this comment.
The service doesn't start. Same thing as if there is a problem loading the dynamic enums.
|
Is the idea that enums thta change more often, e.g. |
Yes, that is what I was thinking. And, Shane has informed me that |
|
I like having the option, and working with actual Scala enums is much easier. Seeing |
| val scalaValues = scalaEnum.enumType.enumValues.map(_.name.toLowerCase) | ||
| s.unique(query.query(_text_enum)).flatMap(l => | ||
| // May need to allow for different ordering and conversions | ||
| if (l === scalaValues) Monad[F].unit |
|
I'm going to take a step back here and reevaluate our approach to dynamic enums. Now that we have them in place, they seem overly complicated to me. They require thinking of the instance and the meta level when programming. Furthermore, in several places we want our code to react to specific instances and referring to them is unsafe, we are assuming their existence and the compiler can't check that. My opinion is to roll back dynamic enums. Let the source of truth be Scala enums. We are building web systems, in part, because they are easy and straightforward to redeploy. So, when we add an enum instance, we can press a button and redeploy everything. If we really want Postgres enums in the DB to represent them, we can generate migrations from the Scala enums. Alternatively, we could have Postgres be the source of truth and generate Scala code from that, like we did before. But I'd rather have Scala be the source of truth for the simple reason that lucuma-odb is upstream in the dependency chain. If we have an "enum" that changes frequently enough that it becomes tedious to redeploy because of it, then maybe it should be its own entity (eg: |
|
I need some time to think about this. Thanks for your patience. |
|
I want to be clear that the purpose of this PR is to spur discussion. Like Raúl, I'm skeptical of the value of the dynamic enums given their complexity, but am amenable to having some of them dynamic if that it seems best. However, I'd prefer if we didn't use dynamic enums for everything. For example, all of the dynamic enums I have created (ProposalStatus, ObsAttachmentType and ProposalAttachmentType) don't really seem to need to be dynamic. I don't care if it is an approach similar to the one in this PR, or something different, but I'd like to be able to use scala enumerations where possible. |
|
FWIW I spent some time figuring out why this was evaluating I think if it had been an |
|
What's the state of this discussion? |
This PR is meant as a topic for discussion.
The "dynamic enums" work well for enumerations that we think might change with some frequency in the future. However, they are more cumbersome to use, especially on the ODB side where we need to treat them as
Tags and convert to enumerations when desired.For enumerations that are unlikely to change, or will change infrequently, the value of the dynamic approach seems questionable. This PR presents a possible alternate solution to making sure that "static" enumerations defined in Scala match the values defined in a Postgres
enumand in the GraphQL API. To me, this seems to be a simpler alternative to the dyamic implementation for enums likeProposalStatus,ObsAttachmentTypeandProposalAttachmentType.