Fix Naming Issues

[lenaten]

Hi,

I just refactor the whole project as described in issue #20 .

There are a lot of changes, and all tests passes expect one, needs help to figure out the reason.

e.g. Travis fail becasue the repo name changed from "graphql-go" to "graphql".

[chris-ramon]

@lenaten I think we should go for ErrFormatted, is a Go convention to use Err prefix for custom errors.

[lenaten]

@chris-ramon there are many things to improve but this pull request is the big one.
let's merge it, and refactor the little things later..

[chris-ramon]

good call, let's handle custom errors renaming on different pr's.

[chris-ramon]

Thanks a lot! 🌟 for going ahead and work on this, appreciate reviews from anyone, thanks!

[sogko]

@lenaten Thanks for taking up the daunting task for renaming / reorganizing the package. 👍🏻
This will take awhile to look through I believe.

My initial thoughts:

1) Tests package name

Since one of the major reasons for this refactor is to have a better API, can I propose that we use the package graphql_test convention for the test packages?

It'll show how graphql will be used as package naturally and serve as a great starting point for any user to quickly dive into the library.
Also, as we work and write tests for the library, we can actually feel and experience the ease/pains of using it.

For e.g. graphql_test.go:

package graphql_test
import (
    "github.com/chris-ramon/graphql"
    "reflect"
    "testing"
)
... // truncated
func TestBasicGraphQLExample(t *testing.T) {
    helloFieldResolved := func(p graphql.GQLFRParams) interface{} {
        return "world"
    }

    schema, err := graphql.NewSchema(graphql.SchemaConfig{
        Query: graphql.NewObject(graphql.ObjectConfig{
            Name: "RootQueryType",
            Fields: graphql.FieldConfigMap{
                "hello": &graphql.FieldConfig{
                    Description: "Returns `world`",
                    Type:          graphql.String,
                    Resolve:     helloFieldResolved,
                },
            },
        }),
    })
    if err != nil {
        t.Fatalf("wrong result, unexpected errors: %v", err.Error())
    }
    query := "{ hello }"
    var expected interface{}
    expected = map[string]interface{}{
        "hello": "world",
    }

    resultChannel := make(chan *Result)
    go graphql.Graphql(graphql.Params{
        Schema:        schema,
        RequestString: query,
    }, resultChannel)
    result := <-resultChannel
    if len(result.Errors) > 0 {
        t.Fatalf("wrong result, unexpected errors: %v", result.Errors)
    }
    if !reflect.DeepEqual(result.Data, expected) {
        t.Fatalf("wrong result, query: %v, graphql result diff: %v", query, Diff(expected, result))
    }

}

Any thoughts on this?

2) Separating internal implementation from public API

Previously we had the following package structure internally:

• gql
• types
• graphqlerrors
• executor
• validator
• languages
  • languages/ast
  • languages/kind
  • languages/lexer
  • languages/location
  • languages/parser
  • languages/printer
  • languages/source
  • languages/visitor

I think this PR change is heading in the right direction with moving in the previous graphqlerrors and types packages into the main graphql package (which was previously gql), as everyone had discussed earlier in #20 and #16 threads.

Can I propose that we separate the internal implementations from the public graphql package?
I would consider executor, validator and languages and most of its sub-packages (such as ast, lexer, parser) as internal implementations.

In other words, 99% of the time, there would be no reason for users to construct and deal with ast structure directly, for example. But those packages can be imported to be used for tooling, for e.g schema_printer, which we then can expose in the main package.

On the other side, we only expose public structs and methods in the graphql package, such as
various GraphQL type structs, error structs and the main Graphql() entry point.

By separating the implementation details and only expose/limit structs/methods that are truly public, we can easily overhaul the internals without worrying about breaking compatibility in the future.
For e.g, we can easily update the executor/parser implementation to use the blazing-fast C/C++ lib parser easily if performance is ever an issue.

Also, it'll be less daunting to maintain than having 60+ files in a flat structure.

So I'm proposing roughly the following package structure:

• graphql
  • graphql.go
  • error.go ... etc (files previously in errors package)
  • scalars.go ... etc (files previously in types package)
  • language folder (previously language package, remains as it is)
  • executor folder (previously executor package, remains as it is)
  • validator folder (previously validator package, remains as it is)

Appreciate any thoughts!

[lenaten]

@sogko thanks for your response.

about the test package name, high level tests should be as your suggest but not the low level tests.
since only graphql_test.go contains high level tests, only this file should be in separate package.
anyway, I think we need a better docs rather than send the developer to read the test files.

about the project structure, I love to see the project structured like you suggested and I think it's should be the next move, but it's more complicated and time consuming thing to do now since it's cause many "cycle import" issues, and we need a clean API as soon as possible.

[sogko]

Hi @lenaten

Appreciate how much work you already put into this 👍🏻

I agree that clean API is necessary and would like to see move forward quickly. But personally, I don't feel easy about having that much files in the package root, I just imagine maintaining it would be a nightmare lol 💀
Plus that would result in exposing internals that would result in the opposite of a cleaner API.

Regarding the import cycles, I see what you mean there. In fact, the structure I initially suggested would have cyclic dependencies (e.g graphql depends on executor, which depends on types defined in graphql; same with validator).

Analysis using dependency graph

I took some time to map out the dependency graph for the current package and the possible moves.
The goal was to find out the minimum set of moves we can make to resolve cyclic imports.
I shared it here https://drive.google.com/file/d/0B3BmZ1veHUNXSU4yYWxYM19FTlU/view?usp=sharing (Warning: get ready to roll up your sleeves and get dirty).

Option 1: Move types into graphql

This was the initial plan as discussed in #20 , i.e move types into graphql.
But that introduces "import cycles" with respect to executor and validator.

Option 2: Move types, executor and validator into graphql

The next best move we can make is to move executor and validator into graphql as well, along with types. That should take care of the cyclic imports.

Option 3: Move errors into graphql

I explored another option to see if we can move errors as well, in case anyone was thinking of it.
We'll end up having to move languages partially into graphql too, to address cyclic imports.
This is not quite ideal.

Option 4: Move everything into graphql

This is the easiest option to avoid any cyclic imports, making graphql a huge monolithic library with >60 files in the root package.
But along with it, some internal components that are implementation-specific gets unnecessarily exposed as well.

We could take this route and decide to do another round of refactoring but I'm not sure if its necessary or productive.

---

Observations

From what I can tell, Option 2 offers the best compromise between

• what we all wanted (i.e. cleaner API by moving types into graphql, along with renaming types as discussed in Naming #20.)
• and minimal set of exposed structs / methods through graphql, (types, executor, validator)

Anyone has any other thoughts? Maybe a different set of eyes might help to see something that I missed.

I know this is getting lengthy and might proved to be frustrating/overwhelming but I hope this would be a worthwhile investment in ensuring we get this done right.

Cheers!

[lenaten]

@sogko I totally agree. thanks for your dependencies analysis, I'm on it..

[lenaten]

@sogko I refactor like your suggestion and it's work fine but some of the tests still have "the cycle import" problem because they depends on graphql.Parse()..

[sogko]

Hi @lenaten

I've pulled your changes locally and took a look at it, and this seems like a good start 👍🏻
Appreciate the amount of work you've put into this!

Notes

Import cycles for tests

I think this is where using the package *_test convention would help to break the cyclic imports.

For e.g. in language/visitor/visitor_test.go, updating the package name of the test file to visitor_test and importing language/visitor should address the issue.
(Note that language/visitor suffers from some stuttering, but since its an internal package, we can fix it in another PR. Maybe we should focus on the main graphql package)

package visitor_test

import (
    ...
    "github.com/chris-ramon/graphql-go"
    "github.com/chris-ramon/graphql-go/language/ast"
    "github.com/chris-ramon/graphql-go/language/visitor"
    ...
)
...
func TestVisitor_AllowsForEditingOnEnter(t *testing.T) {

    query := `{ a, b, c { a, b, c } }`
    astDoc := parse(t, query)

    expectedQuery := `{ a,    c { a,    c } }`
    expectedAST := parse(t, expectedQuery)
    v := &visitor.VisitorOptions{
        Enter: func(p visitor.VisitFuncParams) (string, interface{}) {
            switch node := p.Node.(type) {
            case map[string]interface{}:
                if testGetMapValueString(node, "Kind") == "Field" && testGetMapValueString(node, "Name.Value") == "b" {
                    return visitor.ActionUpdate, nil
                }
            }
            return visitor.ActionNoChange, nil
        },
    }

    editedAst := visitor.Visit(astDoc, v, nil)
    if !reflect.DeepEqual(ASTToJSON(t, expectedAST), editedAst) {
        t.Fatalf("Unexpected result, Diff: %v", Diff(expectedAST, editedAst))
    }

}

language/parser

Regarding the language/parser, I assumed that you moved it into graphql because visitor_test.go was giving you a cyclic import error when it was in visitor package?

I believe once you break the cyclic dependency for visitor, as stated above, you should not face an issue with moving language/parser package back.

gqlerrors

Regarding name of the gqlerrors package, I'm going to defer the decision to @chris-ramon .
I'm not sure whether the gql abbreviation is still being considered since the plan is to move to graphql for the package name.

Possible options on the table

• graphqlerrors, maintain it as it was previously
• gqlerrors, as proposed by @lenaten in this PR. (Note: I previously did suggested this name so as to be consistent with the previously considered gqltypes, but since then, it seems like we are not heading that way anymore)
• errors, I'm playing devil's advocate here and just put this out there for the sake of discussion. It's the shortest, most concise package name lol. (But this hijacks the built-in errors package, forcing users to alias it 99% of the time. Not ideal.)

testutils

I intentionally left out testutils package from the dep-graph analysis previously because its only meant to be consumed by the test packages and not part of the main library.

May I suggest that we restore testutils package back as a separate entity, instead of merging it into graphql root? I worry that we'd be exposing test helper functions (such asTestParse(), Diff() etc) unnecessarily through graphql.
This means that some of the tests have to revert back to using the package *_test convention (for .e.g abstract_test.go, directives_test.go) to have proper package dependencies.

---

Again, thanks for donating your time to contribute to this. I can appreciate the effort you spend, and I'm sure @chris-ramon feels that same way too.

Cheers!

[lenaten]

@sogko actually your solution will work but I don't understand why tests of one package needs to be dependent on another package? maybe it's a sign of unnecessary coupling?

[sogko]

Hi @lenaten

Besides being a wonderful go convention, having separate packages (for e.g. package graphql and package graphql_test) actually have the effect of reducing coupling between test code and library code, and defines clearer boundaries between library (graphql), test land, and user (i.e. users of this library). Once we have these clear boundaries, it'll be easier to see if there is unnecessary couplings.

So test codes naturally depends on library code, but not the other way around. (i.e. Library code should not have access to test structs and methods.)

For e.g

• Packages in library space should never depend on packages in test space or user space
• Packages test space and user space may depend on packages in library space, along with other libraries they may need. For example,
  • graphql_server_1 written by user1, depends on graphql only
  • graphql_server_2 written by user2, depends on graphql and errors, probably to handle some custom errors.
  • If another user wants to delve into the internals of the language by importing one of the languages sub-packages, she may do so, but not through graphql, as intended.
• We naturally want to discourage users from depending on packages in test space. They are more brittle and something that we do not want to provide support for outside of this package.
  By putting it under graphql_test package, for example, users won't have access to Test*() and other test structs when they import github.com/chris-ramon/graphql.

---

Black-box vs white-box

In addition to that, the decision to put the test in the same package or in another _test package depends on the type of test we want to perform: black-box testing vs white-box testing.
There's nothing wrong with using one or the other, or even both. It depends on what we need.

For black-box testing:
The test code are written in a separate _test package, and the tests are meant to only to use the exported structs and methods.
It make sense to use black-box for most of the stuff we want to test in graphql package, for e.g. graphql.Graphql(), graphql.NewSchema() etc, which are meant to be public APIs.

The tests won't have access to internal implementations, exactly what users of the library should not have access either.

For white-box testing:
When we need to test complex internal function that require access to the internals, so that make sense to put in the same package. Probably some internal stuff in languages could use this approach.

I am not saying that we can't write the tests for exported APIs in white-box fashion, it is possible.

But enforcing that the public APIs are written in black-box tests, we can have better assurances on the quality and allows for better maintainability.

---

Testing for usability

You can look at the way the tests were written this way: besides testing for the correctness of a particular API, we also want to test its usability.

Consider the following:

package graphql

func TestForSomething(...) {
    go Graphql(GraphqlParams{
        Schema:        schema,
        RequestString: query,
    }, resultChannel)
    ...
}

// vs

package graphql_test

import (
    ...
    "github.com/chris-ramon/graphql-go"
)
func TestForSomething(...) {
    go graphql.Graphql(graphql.GraphqlParams{
        Schema:        schema,
        RequestString: query,
    }, resultChannel)
    ...
}

Both of the test are correct, but the second one allows us to actually use the library as we write the test. We can then feel the same excitement or worst, the same pains, as the user of this library.
Straight away, while writing the above test, you can feel that you would be typing Graphql one too many times.
(That was how we found out about the need to update the API and work towards making it better).

We could also see what imports the user needs to make in order to achieve something. (For e.g, previously we saw that every time user imports graphql-go, they always needed to import types as well. Thats where the decision to merge types into graphql came from.)

Share your thoughts on this 👍🏻

Cheers!

[sogko]

@lenaten I've submitted a PR on your fork (lenaten#1) with the fix for the failed tests and restored the language/parser and testutil.

If you could kindly take a look and merge it into your lenaten:naming branch =)

Hopefully this can move the PR along a bit faster 👍🏻

Cheers!
/cc @chris-ramon

[chris-ramon]

Awesome! thanks a lot for ur work on this guys! 🌟

Def agree on have black-box testing and changes looks good to me 👍

In regards renaming gqlerrors I think for consistency we might want to rename it to graphqlerrors, if so we can handle that on new PR.

If this one is ready, feel free to merge it @sogko.