Introduce a descriptor sum type to better reflect the domain logic

[mdimjasevic]

This patch updates the descriptor definition type DescriptorDefinition by turning it from a record type with multiple Maybe values into a sum type such that these Maybe values are replaced with proper values in each data constructor. Until now a user was able to provide a configuration that made no sense in the domain.

This change reflects the domain logic that an inner descriptor cannot have a rate limit and that it must have sub-descriptors, and conversely, that a leaf descriptor must have a rate limit and it must have no sub-descriptors. Two new tests are added to the Fencer.Types.Test module to confirm the negative cases: test_parseJSONFaultyDescriptorDefinition1 and test_parseJSONFaultyDescriptorDefinition2.

A suggested order of reviewing:

1. lib/Fencer/Types.hs
2. lib/Fencer/Rules.hs
3. test/Fencer/Rules/Test/Helpers.hs
4. test/Fencer/Types/Test.hs
5. test/Fencer/Rules/Test/Examples.hs
6. test/Fencer/Logic/Test.hs

This closes issue #111.

[effectfully]

I'm generally in favor of these changes, but I advice to get rid of partial fields.

[effectfully]

I'm personally not a fan of partial fields. In this case I think I'd probably abstract keys and values out and define this data type as follows:

data DescriptionContent
    = DescriptionContentDescriptors  ![DescriptorDefinition]
    | DescriptionContentRateLimit !RateLimit

data DescriptionDefinition a = DescriptionDefinition
    { descriptorDefinitionKey :: !RuleKey
    , descriptorDefinitionValue :: !(Maybe RuleValue)
    , descriptorDefinitionDescriptors :: !DescriptionContent
    }

I believe this is a better definition.

[mdimjasevic]

The old version (i.e., the present in master) allows specifying a descriptor that has both a rate limit and a list of sub-descriptors. This doesn't make sense in the domain logic. Your proposal suffers from the same problem, and I'd argue it makes it even worse - it makes it mandatory to specify values that don't make sense in the domain logic. Or am I missing something?

[srid]

I actually wondered about this too.

This doesn't make sense in the domain logic. Your proposal suffers from the same problem

Hmm, I don't see how? A leaf descriptor would be defined as,
DescriptionDefinition ruleKey (Just ruleVal) (DescriptionContentRateLimit rateLimit)

and a non-leaft descriptor would be defined as
DescriptionDefinition ruleKey (Just ruleVal) (DescriptionContentDescriptors [...])

Right?

[effectfully]

The old version (i.e., the present in master) allows specifying a descriptor that has both a rate limit and a list of sub-descriptors. This doesn't make sense in the domain logic. Your proposal suffers from the same problem

No, it doesn't. My version is isomorphic to yours. Note that DescriptionContent is a sum type just like your DescriptionDefinition, I just extracted common data from the constructors and encoded the choice between descriptors and rate limit as a dedicated data type.

[mdimjasevic]

Oh, my apologies, @effectfully! I wasn't reading carefully enough and I thought that your DescriptionContent is a product type when in fact it is a sum type.

[effectfully]

Again, I'd remove partial fields and turn them into haddocks. Whenever you want to extract a value from a sum type, you have to handle the other alternatives and partial fields sweep that requirement under the carpet, which can easily lead to bugs.

[mdimjasevic]

@effectfully, would the following be a good solution for the partial fields problem you brought up?

Instead of using a sum type where data constructors are given as records, I'd define a GADT. To keep accessors for common fields to all data, I can simply write an accessor function for each such field; in the case of descriptor definitions, there would be one for the key and one for the optional value.

See commit c4c16a9 for the actual implementation of the proposal.

[effectfully]

Instead of using a sum type where data constructors are given as records, I'd define a GADT.

It's completely fine to use sum types, surely I'm not advocating against them. The code that I've written above has a sum type. A GADT is an unnecessary overcomplication and I believe you should revert that commit. I'm not proposing to get rid of sum types, I'm only proposing to get rid of partial fields:

The option -Wpartial-fields warns about record fields that could fail when accessed via a lacking constructor. The function f below will fail when applied to Bar, so the compiler will emit a warning at its definition when -Wpartial-fields is enabled.

data Foo = Foo { f :: Int } | Bar

I.e. in the example above I'd suggest to remove f, not to restructure Foo. And this is precisely what I propose with RuleBranch: just remove the partial accessors (ruleBranchNested and ruleBranchRateLimit) without touching anything else.

In DescriptionDefinition I suggest to restructure things a bit, but note that my version is isomorphic to yours, I just find my version more ergonomic.

[mdimjasevic]

Thank you both for all the clarifications!

At first I thought that partial fields are at odds with sum types, but in fact partial fields are at odds with sum types where data constructors are records.

@effectfully, I realized that GADTs are an overkill for this task and I replaced them with plain ADTs. In the current version I have no records, but if you think it'd be better to get them back, let me know and I'll do that; otherwise I'm merging with plain ADTs, like below:

-- | Config for a single rule tree.
data DescriptorDefinition
  =
    -- | An inner node with no rate limit
    DescriptorDefinitionInnerNode
      !RuleKey
      !(Maybe RuleValue)
      ![DescriptorDefinition]

    -- | A leaf node with a rate limit
  | DescriptorDefinitionLeafNode
      !RuleKey
      !(Maybe RuleValue)
      !RateLimit
  deriving stock (Eq, Show)

descriptorDefinitionKey
  :: DescriptorDefinition
  -> RuleKey
descriptorDefinitionKey (DescriptorDefinitionInnerNode k _ _) = k
descriptorDefinitionKey (DescriptorDefinitionLeafNode  k _ _) = k

descriptorDefinitionValue
  :: DescriptorDefinition
  -> Maybe RuleValue
descriptorDefinitionValue (DescriptorDefinitionInnerNode _ v _) = v
descriptorDefinitionValue (DescriptorDefinitionLeafNode  _ v _) = v

-- | A single branch in a rule tree, containing several (or perhaps zero)
-- nested rules.
data RuleBranch
  = RuleBranch !RuleTree
  | RuleLeaf !RateLimit