dillonkearns / elm-graphql

Autogenerate type-safe GraphQL queries in Elm.
https://package.elm-lang.org/packages/dillonkearns/elm-graphql/latest
BSD 3-Clause "New" or "Revised" License
778 stars 107 forks source link

Requesting feedback on the following changes to the serializeChildren function regarding field hashing #595

Closed hariroshan closed 2 years ago

hariroshan commented 2 years ago

Hello, I have made the following changes to the serializeChildren function to hash the fields on the query

serializeChildren : Maybe Int -> List RawField -> String
serializeChildren indentationLevel children =
    children
        |> mergedFields
        |> nonemptyChildren
        |> canAllowHashing -- Added this function 
        |> List.map
            (\( field, maybeAlias ) ->
                serialize maybeAlias (indentationLevel |> Maybe.map ((+) 1)) field
            )
        |> List.filterMap identity
        |> String.join
            (case indentationLevel of
                Just _ ->
                    "\n"

                Nothing ->
                    " "
            )

canAllowHashing : List RawField -> List ( RawField, Maybe String )
canAllowHashing rawFields =
    let
        isAllDifferent =
            rawFields
                |> List.map
                    (\f ->
                        case f of
                            Leaf { fieldName } _ ->
                                fieldName

                            Composite fieldName _ _ ->
                                fieldName
                    )
                |> List.Extra.allDifferent
    in
    rawFields
        |> List.map
            (\field ->
                ( field
                , if isAllDifferent then
                    Nothing

                  else
                    alias field
                )
            )

The above mentioned changes will only add hashed field alias only when there's a duplicate field name. And In the decoder, the following changes were made


{-| Refer to a field in auto-generated code.
-}
selectionForField : String -> String -> List Argument -> Decoder decodesTo -> SelectionSet decodesTo lockedTo
selectionForField typeString fieldName args decoder =
    let
        newLeaf =
            leaf { typeString = typeString, fieldName = fieldName } args
    in
    SelectionSet [ newLeaf ]
        (Decode.oneOf -- Changed
            [ Decode.field (Graphql.Document.Field.hashedAliasName newLeaf) decoder
            , Decode.field fieldName decoder
            ]
        )

{-| Refer to an object in auto-generated code.
-}
selectionForCompositeField :
    String
    -> List Argument
    -> SelectionSet a objectScope
    -> (Decoder a -> Decoder b)
    -> SelectionSet b lockedTo
selectionForCompositeField fieldName args (SelectionSet fields decoder) decoderTransform =
    SelectionSet [ composite fieldName args fields ]
        (Decode.oneOf -- Changed
            [ Decode.field
                (Graphql.Document.Field.hashedAliasName (composite fieldName args fields))
                (decoderTransform decoder)
            , Decode.field fieldName (decoderTransform decoder)
            ]
        )

These changes seems to work as expected. for example the serialized SelectionSet looks like following,

query {
  human1213318493: human(id: "1001") {
    name
  }
  human3685532794: human(id: "1004") {
    name
  }
  hero {
    id
    name
    appearsIn
    friends {
      name
    }
    __typename
    ...on Human {
      homePlanet
    }
    ...on Droid {
      primaryFunction
    }
  }
}

and decodes fine

Ok ( Just { name = "Darth Vader" }
      , Just { name = "Wilhuff Tarkin" }
      , { appearsIn = [ Newhope, Empire, Jedi ]
        , details = HumanDet (Just "Tatooine")
        , friends = [ "Han Solo", "Leia Organa", "C-3PO", "R2-D2" ]
        , id = Id "1000"
        , name = "Luke Skywalker"
        }
      )

Is there any edge case for this change? If it seems ok, can we merge these with the project?

dillonkearns commented 2 years ago

Fascinating! That's a clever trick to try both ways in Decode.oneOf.

It seems like a solid approach to me, I can't think of any edge cases. I think it looks good. Thanks for opening an issue first to discuss and giving some good background! I'd be happy to review and merge a PR for this.

Feel free to reach out to me if there's anything I can help with.

hariroshan commented 2 years ago

I have made the commits. Can you merge?