search

haskell-graphql / graphql-api

Write type-safe GraphQL services in Haskell
BSD 3-Clause "New" or "Revised" License
406 stars 35 forks source link

Go through TODOs & XXXs in code for genuine bugs #80

Closed jml closed 7 years ago

jml commented 7 years ago

Worth doing a quick trawl before release. File things that are behaviour-affecting bugs. Fix them if they are important.

jml commented 7 years ago

From 6c4a6f49511511ddbac40b41f82405ecff2215ac

$ git todo -f .
src/GraphQL.hs:89:
  -- TODO: Prevent this at compile time.

src/GraphQL/API.hs:77:
  -- TODO(tom): AFACIT We can't constrain "fields" to e.g. have at least
  -- one field in it - is this a problem?

src/GraphQL/API.hs:258:
  -- See TODO comment in "HasAnnotatedType" class for nullability.

src/GraphQL/API/Enum.hs:23:
  -- TODO: Enums have a slightly more restricted set of names than 'Name'
  -- implies. Especially, they cannot be 'true', 'false', or 'nil'. The parser
  -- /probably/ guarantees this, so it should export this guarantee by providing
  -- an 'Enum' type.

src/GraphQL/API/Enum.hs:30:
  -- XXX: Why is this 'Text' and not 'NameError'?

src/GraphQL/API/Enum.hs:58:
  -- XXX: This is impossible to catch during validation, because we cannot
  -- validate type-level symbols, we can only validate values. We could
  -- show that the schema is invalid at the type-level and still decide to
  -- call this anyway. The error should rather say that the schema is
  -- invalid.
  --
  -- Further, we don't actually have any schema-level validation, so
  -- "should have been caught during validation" is misleading.

src/GraphQL/Internal/Output.hs:92:
  -- XXX: 1-indexed natural number
  -- XXX: 1-indexed natural number

src/GraphQL/Internal/Schema.hs:41:
  -- XXX: Use the built-in NonEmptyList in Haskell

src/GraphQL/Internal/Schema.hs:161:
  -- XXX: spec is unclear about default value for input object field definitions

src/GraphQL/Internal/Syntax/AST.hs:114:
  -- TODO: Just make Node implement HasName.

src/GraphQL/Internal/Syntax/Encoder.hs:97:
  -- TODO: This will be replaced with `decimal` Buidler

src/GraphQL/Internal/Syntax/Encoder.hs:99:
  -- TODO: This will be replaced with `decimal` Buidler

src/GraphQL/Internal/Syntax/Encoder.hs:112:
  -- TODO: Escape characters

src/GraphQL/Internal/Syntax/Parser.hs:103:
  -- TODO: Make sure it fails when `... on`.
  -- See https://facebook.github.io/graphql/#FragmentSpread

src/GraphQL/Internal/Syntax/Parser.hs:151:
  -- TODO: Handle maxBound, Int32 in spec.

src/GraphQL/Internal/Validation.hs:41:
  -- TODO, can we hide this again?

src/GraphQL/Internal/Validation.hs:119:
  -- | Get the selection set for an operation.
  --
  -- TODO: This doesn't return the *actual* selection set we need, because it
  -- hasn't substituted variables or applied directives.

src/GraphQL/Internal/Validation.hs:187:
  -- TODO: Either make operation type (Query, Mutation) a parameter of an
  -- Operation constructor or give all the fields accessors. This duplication is
  -- driving me batty.

src/GraphQL/Internal/Validation.hs:253:
  -- | Get all of the fields directly inside the given selection set.
  --
  -- TODO: This ignores fragments, whereas it should actually do something with
  -- them.
  --
  -- TODO: At this point, we ought to know that field names are unique. As such,
  -- we should return an ordered map of Name to Fields, rather than a bland
  -- list.

src/GraphQL/Internal/Validation.hs:408:
  -- TODO: Check that the fields are mergable.

src/GraphQL/Internal/Validation.hs:582:
  -- TODO: There's a chunk of duplication around "this collection of things has
  -- unique names". Fix that.

src/GraphQL/Internal/Validation.hs:585:
  -- TODO: Might be nice to have something that goes from a validated document
  -- back to the AST. This would be especially useful for encoding, so we could
  -- debug by looking at GraphQL rather than data types.

src/GraphQL/Resolver.hs:24:
  -- TODO (probably incomplete, the spec is large)
  -- - input objects - I'm not super clear from the spec on how
  --   they differ from normal objects.
  -- - "extend type X" is used in examples in the spec but it's not
  --   explained anywhere?
  -- - Directives (https://facebook.github.io/graphql/#sec-Type-System.Directives)
  -- - Enforce non-empty lists (might only be doable via value-level validation)

src/GraphQL/Resolver.hs:74:
  -- | Found duplicate fields in set.
  -- TODO: Catch this in validation
  -- | We tried to use an inline fragment with a name that the union
  -- type does not support.

src/GraphQL/Resolver.hs:92:
  -- TODO: format 'result' nicely

src/GraphQL/Resolver.hs:183:
  -- TODO check that selectionset is empty (we expect a terminal node)

src/GraphQL/Resolver.hs:190:
  -- TODO check that selectionset is empty (we expect a terminal node)

src/GraphQL/Resolver.hs:195:
  -- TODO check that selectionset is empty (we expect a terminal node)

src/GraphQL/Resolver.hs:200:
  -- TODO check that selectionset is empty (we expect a terminal node)

src/GraphQL/Resolver.hs:219:
  -- TODO: A parametrized `Result` is really not a good way to handle the
  -- "result" for resolveField, but not sure what to use either. Tom liked the
  -- tuple we had before more because it didn't imply any other structure or
  -- meaning. Maybe we can just create a new datatype. jml thinks we should
  -- extract some helpful generic monad, ala `Validator`.

src/GraphQL/Resolver.hs:226:
  -- Extract field name from an argument type. TODO: ideally we'd run
  -- this directly on the "a :> b" argument structure, but that requires
  -- passing in the plain argument structure type into resolveField or
  -- resolving "name" in the buildFieldResolver. Both options duplicate
  -- code somwehere else.

src/GraphQL/Resolver.hs:406:
  -- TODO(tom): we're getting to a point where it might make sense to
  -- split resolver into submodules (GraphQL.Resolver.Union  etc.)

src/GraphQL/Resolver.hs:527:
  -- TODO(tom) - we might want to move to Typeable `cast` for uValue
  -- instead of doing our own unsafeCoerce because it comes with
  -- additional safety guarantees: Typerep is unforgeable, while we
  -- can still into a bad place by matching on name only. We can't
  -- actually segfault this because right now we walk the list of
  -- objects in a union left-to-right so in case of duplicate names we
  -- only every see one type. That doesn't seen like a great thing to
  -- rely on though!

src/GraphQL/Value.hs:109:
  -- TODO: These next two definitions are quite internal. We should move this
  -- module to Internal and then re-export the bits that end-users will use.

src/GraphQL/Value.hs:240:
  -- TODO: GraphQL does not allow heterogeneous lists:
  -- https://facebook.github.io/graphql/#sec-Lists, so this will generate
  -- invalid lists.

src/GraphQL/Value/FromValue.hs:117:
  -- TODO(tom): How do we deal with optional fields? Maybe sounds
  -- like the correct type, but how would Maybe be different from
  -- `null`? Delegating to FromValue not good enough here because of
  -- the dictionary lookup.

src/GraphQL/Value/ToValue.hs:19:
  -- XXX: Should this just be for Foldable?

src/GraphQL/Value/ToValue.hs:23:
  -- TODO - tom still thinks that using Maybe for nullable is maybe not
  -- the best idea.

src/GraphQL/Value/ToValue.hs:44:
  -- XXX: Make more generic: any string-like thing rather than just Text.

tests/EndToEndTests.hs:262:
  -- TODO: This error message is pretty bad. We should define
  -- a typeclass for client-friendly "Show" (separate from
  -- actual Show which remains extremely useful for debugging)
  -- and use that when including values in error messages.

tests/EndToEndTests.hs:272:
  -- TODO: This is a crummy way to make a variable map. jml doesn't want
  -- to come up with a new API in this PR, but probably we should have a
  -- very simple function to turn a JSON value / object into the
  -- variable map that we desire. Alternatively, we should have APIs
  -- like Aeson does.

tests/ExampleSchema.hs:88:
  -- XXX: This really shouldn't be part of Resolver, since whether or not a
  -- thing has a default is part of the API / Schema definition.

tests/ExampleSchema.hs:111:
  -- TODO: Probably shouldn't have to do this for enums.

tests/ExampleSchema.hs:235:
  -- TODO: Extend example to cover unions, interfaces and lists by giving humans
  -- a list of pets and a list of cats & dogs.

tests/Examples/InputObject.hs:16:
  -- TODO defaultFor takes a Name which makes sense, but what's the
  -- name for an input object?

45 TODOs spread across:

$ git todo -f . | grep '^[a-zA-Z]' | sed -e 's/:.*$//' | uniq -c
      1 src/GraphQL.hs
      2 src/GraphQL/API.hs
      3 src/GraphQL/API/Enum.hs
      1 src/GraphQL/Internal/Output.hs
      2 src/GraphQL/Internal/Schema.hs
      1 src/GraphQL/Internal/Syntax/AST.hs
      3 src/GraphQL/Internal/Syntax/Encoder.hs
      2 src/GraphQL/Internal/Syntax/Parser.hs
      7 src/GraphQL/Internal/Validation.hs
     11 src/GraphQL/Resolver.hs
      2 src/GraphQL/Value.hs
      1 src/GraphQL/Value/FromValue.hs
      3 src/GraphQL/Value/ToValue.hs
      2 tests/EndToEndTests.hs
      3 tests/ExampleSchema.hs
      1 tests/Examples/InputObject.hs
jml commented 7 years ago

38 TODO spread across:

$ git todo -f . | grep '^[a-zA-Z]' | sed -e 's/:.*$//' | uniq -c
      1 src/GraphQL.hs
      2 src/GraphQL/API.hs
      3 src/GraphQL/API/Enum.hs
      1 src/GraphQL/Internal/Output.hs
      3 src/GraphQL/Internal/Schema.hs
      1 src/GraphQL/Internal/Syntax/AST.hs
      3 src/GraphQL/Internal/Syntax/Encoder.hs
      2 src/GraphQL/Internal/Syntax/Parser.hs
      4 src/GraphQL/Internal/Validation.hs
      5 src/GraphQL/Resolver.hs
      2 src/GraphQL/Value.hs
      1 src/GraphQL/Value/FromValue.hs
      3 src/GraphQL/Value/ToValue.hs
      3 tests/EndToEndTests.hs
      3 tests/ExampleSchema.hs
      1 tests/Examples/InputObject.hs

Yay us.