Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test that descriptor key can not be empty #95

Open
neongreen opened this issue Nov 8, 2019 · 2 comments
Open

Test that descriptor key can not be empty #95

neongreen opened this issue Nov 8, 2019 · 2 comments

Comments

@neongreen
Copy link
Contributor

neongreen commented Nov 8, 2019

https://github.com/lyft/ratelimit/blob/master/test/config/config_test.go#L185-L194

@mdimjasevic
Copy link
Contributor

mdimjasevic commented Nov 27, 2019

While it would be possible to test this, I'd argue there is not much point in this because the test would hold vacuously. We have this guarantee from Haskell's type checker. This is the definition of the descriptor:

fencer/lib/Fencer/Types.hs

Lines 154 to 161 in 8d627af

-- | Config for a single rule tree.
data DescriptorDefinition = DescriptorDefinition
{ descriptorDefinitionKey :: !RuleKey
, descriptorDefinitionValue :: !(Maybe RuleValue)
, descriptorDefinitionRateLimit :: !(Maybe RateLimit)
, descriptorDefinitionDescriptors :: !(Maybe [DescriptorDefinition])
}
deriving stock (Eq, Show)

and this is its FromJSON instance:

fencer/lib/Fencer/Types.hs

Lines 177 to 183 in 8d627af

instance FromJSON DescriptorDefinition where
parseJSON = withObject "DescriptorDefinition" $ \o -> do
descriptorDefinitionKey <- o .: "key"
descriptorDefinitionValue <- o .:? "value"
descriptorDefinitionRateLimit <- o .:? "rate_limit"
descriptorDefinitionDescriptors <- o .:? "descriptors"
pure DescriptorDefinition{..}

In other words, key has to be there, otherwise we get an exception like this:

2019-11-27T16:39:31Z, E, error loading new configuration from runtime: "nix/ratelimit/ratelimit/config/another.yaml", Aeson exception:
Error in $.descriptors[0].key: parsing Text failed, expected String, but encountered Null

@mdimjasevic
Copy link
Contributor

@neongreen: Can I close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants