Summary

This pull request adds functionality to use Github GraphQL API and replaces REST API with GraphQL API where pull request details are retrieved.

The new graphql package communicates with GraphQL API via https://github.com/shurcooL/githubv4. Anything related to this third party package are encapsulated for decoupling reasons:

• Query struct needed by githubv4 can be created in Go code with the new graph/query.
• Response from githubv4 is converted to a local type Response (server/plugin/internal/graphql/response.go). All githubv4 types are converted to native data types.

Ticket Link

https://github.com/mattermost/mattermost-plugin-github/issues/325

Hello @srgyrn,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Codecov Report

Merging #404 (cd7f26e) into master (4b4bc21) will increase coverage by 5.08%. The diff coverage is 49.89%.

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   19.34%   24.42%   +5.08%     
==========================================
  Files          11       21      +10     
  Lines        2766     3132     +366     
==========================================
+ Hits          535      765     +230     
- Misses       2193     2305     +112     
- Partials       38       62      +24     

Impacted Files	Coverage Δ
server/plugin/api.go	8.12% <0.00%> (+0.66%)	:arrow_up:
server/plugin/internal/graphql/client.go	0.00% <0.00%> (ø)
server/plugin/plugin.go	4.62% <0.00%> (-0.22%)	:arrow_down:
server/plugin/search/pullrequest.go	0.00% <0.00%> (ø)
server/plugin/search/queries.go	0.00% <0.00%> (ø)
server/plugin/internal/graphql/query/union.go	62.50% <62.50%> (ø)
server/plugin/internal/graphql/response.go	63.33% <63.33%> (ø)
server/plugin/internal/graphql/query/builder.go	72.34% <72.34%> (ø)
server/plugin/internal/graphql/query/node.go	87.50% <87.50%> (ø)
server/plugin/internal/graphql/query/scalar.go	88.23% <88.23%> (ø)
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b4bc21...dccb242. Read the comment docs.

@srgyrn That you very much for raising this PR! Given the sheer size I will need a few days to properly review it.

@reflog I'm wondering if you are interested in also reviewing this PR as your worked with GraphQL in the past IIRC.

@srgyrn thanks for your contribution! Before we dive into the review - I wanted to ask - what prompted it? What are we trying to fix? It seems (from a cursory glance at the code) that quite alot of new code was added, so I want to understand the benefit first.

Thanks again!

@reflog I was there when the ticket was created. The main problem is that for fetching all the needed information for the PRs and issues we now have to do 3 different calls for each open issue assigned. That can consume the easily the allowed rates and takes significant time to load.

Using GraphQL potentially will solve this, since we are able to get the whole information we need in just one query.

You can get a bit more of insight in the linked GH issue.

@larkox - awesome. Thanks for the details. I'll get on the review

@srgyrn question - what is the reason for implementing so many 'generic' graphql methods, when all we need to do is to construct a struct with proper tags, like:

var q struct {
	RepositoryOwner struct {
		Login        string
		Organization struct {
			Description string
		} `graphql:"... on Organization"`
		User struct {
			Bio string
		} `graphql:"... on User"`
	} `graphql:"repositoryOwner(login: \"github\")"`
}

and execute it? I don't see the reason for having this 'query builder' pattern and scalar related stuff in this plugin.

Hey @reflog,

Having worked in projects before which communicate with Elasticsearch without a client like Elastica, I've seen codebases end up getting polluted with so many, so long and sometimes redundant json queries. Also, using the structs expected by githubv4 within the project's code will make the project very dependent to this third party package. You can see in the example structs given in githubv4's repo, package specific data types like githubv4.String, githubv4.HTML or enums are used. So anyone who wants to contribute to the project will have to know and/or learn about them too.

True, my suggested way makes query creating longer and involves more error checks than just writing down a struct. However, it leaves the project's dependency on a third party package just at the query builder bit. Creating scalar queries is all about copying and pasting names and values from the GraphQL Explorer. More complex queries (object, union...) is all about setting children in them.

With the query builder, you don't need to worry about package specific data types at all once required ones are added to the decorator/scalartypes.go. The same thing goes for struct tags as well. You can simply add a new one with validations so there won't be an unnecessary call to the GraphQL API if an unexpected value is set.

@srgyrn - in general, I agree with you. Having better abstractions is great for large projects. This is however seems to me like an overkill for this plugin. But I am about 0/5 on this. Let's pull a couple more people in for the discussion. cc @ethervoid @agnivade

I really don't have any knowledge about GraphQL, but from a software design perspective - the tradeoff here is codebase size vs too many abstractions. For a big project that heavily uses GraphQL queries, it makes sense to write a low-level library to manage boilerplates in a proper way. But for a small project which uses GraphQL sparingly, I would lean towards using the structs to keep things simple and maintainable.

There is a cost to more code that we add. That has more chance of bugs, more lines of code to maintain and debug. The benefits of additional code should outweigh these costs.

I really don't have much context on the future direction that this plugin will take or how much more GraphQL might be used in future, so I will leave it to the plugin owners to take a call on that.

@srgyrn - In any case, it is wonderful to see the effort that you have put in to write so much code and also tests for it. Thank you for your time :+1:

First of all, I love the work you've done here, and seeing the number of tests you made to cover it is amazing. Thank you very much

Regarding the PR itself, I agree with you in the base, it's really useful to decouple third-party libraries and I like the idea but I also agree with the comment from other reviewers, this looks a little overkill for this plugin.

If I may suggest I'd do something like this. Decouple the library using a service so we can mock that up and test it properly. You can also encapsulate the queries inside and in case the situation explodes as you said from past experiences we can connect a new "Client" and use the builder.

Again I want to stress that I think this is amazing work and has a lot of effort poured into it. Thank you

@reflog @agnivade @ethervoid Thank you for your time and feedback :)

@ethervoid Shall I remove the query builder and integrate the package like the example you've suggested or close the PR?

@srgyrn thank you! Please don't close the PR, just simplify it if possible. @ethervoid suggestion sounds good

Agree with @reflog , you can work on this PR :) . Thank you!

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

Can we get a consensus or proposal from the reviewers that will provide actionable items for the PR author.

Thanks for your work, @srgyrn

The suggested proposal is right here https://github.com/mattermost/mattermost-plugin-github/pull/404#issuecomment-732816812.

Hello everyone, I've been a bit busy at work but started working on the requested changes this weekend. I should be pushing them next week. I'm sorry I haven't been responsive enough.

Hello @hanzei, I did the requested changes. The golangci-lint fails but it seems to be a false positive:

• https://github.com/golangci/golangci-lint/issues/1517
• https://gitlab.com/opennota/check#known-limitations

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@hanzei, gentle ping for review :)

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@srgyrn, some folks are just getting back from a break but will get to this shortly. @hanzei, gentle reminder to review

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@DHaussermann may you please take a look and finish the review cycle? Thx!

@srgyrn Awesome work! This is a great performance improvement and simplifies the communication with GitHub. I have one comment on the functionality behavior.

Before this PR, the functionality worked like:

• The user visits a tab on the LHS such as "Your Open Pull Requests"
• The frontend sends a request to fetch PR details and provides a list of PR number / repo pairs (specific to the current tab's category)
• The backend performs an HTTP request per each pair

Now the backend is using GraphQL to fetch all at once, but it is not taking into account the pairs being sent by the client. Is the backend fetching/returning more than necessary in the GraphQL query? Or is this negligible due to the dataset generally being on the same order of size across each tab category? cc @hanzei

/update-branch

/update-branch