Provide Go client library #141

minwoox · 2018-03-05T09:50:21Z

Motivation:
If we provide Go client library, more people can adapt Central Dogma.

Modifications:

Add Go client library

Result:

More customers

Todos:

Repackage the original sources
Adapt CLI using this library
Add the watcher

trustin · 2018-03-08T03:41:07Z

client/go/dogma/content_service_test.go

+	_ = &Change{Path: "/a.json", Type: "APPLY_JSON_PATCH", Content: content}
+
+	// TODO(minwoox) fix this to work
+	//if !reflect.DeepEqual(entry, want) {


Will you fix this in this PR or?

trustin · 2018-03-08T03:43:14Z

client/go/dogma/content_service_test.go

+		{Path: "/b.txt", Type: "APPLY_TEXT_PATCH", Content: "--- /b.txt\n+++ /b.txt\n@@ -1,1 +1,1 @@\n-foo\n+bar"}}
+
+	// TODO(minwoox) fix this to work
+	//if !reflect.DeepEqual(changes, want) {


trustin · 2018-03-08T03:43:59Z

client/go/dogma/dogma.go

+	DefaultPort       = 36462
+	DefaultPathPrefix = "api/v1/"
+	PathPrefixV0      = "api/v0/"
+	DefaultBaseURL    = "http://localhost:36462/"


Just curious - can we construct this from DefaultScheme, DefaultHostName and DefaultPort?

Perhaps we could make all constants here except DefaultPort hidden from the public API?

Fixed. Thank you ~!
I thought I couldn't make it because strconv.Itoa(port) didn't work. But I found that I can simply do string(port).

trustin · 2018-03-08T03:47:33Z

client/go/dogma/dogma.go

+	return NewClient(baseURL, config.Client(context.Background()))
+}
+
+func NewClient(baseURL string, client *http.Client) (*Client, error) {


Which would be used more frequently? If it's NewClientWithCredential, we could rename NewClientWithCredential to NewClient and NewClient to NewClientWithHTTPClient?

Fixed. Thank you~!

trustin · 2018-03-08T03:48:23Z

client/go/dogma/dogma.go

+	defer res.Body.Close()
+
+	if res.StatusCode != http.StatusOK {
+		return false, fmt.Errorf("authenticaion check is failed (status: %s)", res.Status)


authentication failed (status: %s)

trustin · 2018-03-08T05:18:44Z

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/AccessToken.java

+
+    private final String refreshToken;
+
+    //TODO(minwoox) add scope when it needs


Add scope if needed. ?

As a part of the response, there is a scope.

scope (optional) If the scope the user granted is identical to the scope the app requested, this parameter is optional. If the granted scope is different from the requested scope, such as if the user modified the scope, then this parameter is required.

in here
But I don't think we need it right now, so just left a comment.

I actually meant to update the comment to 'Add scope if needed.' rather than asking to add it in this PR. :-D

Oops~ got it.~! Thank you!

trustin · 2018-03-08T05:20:53Z

common/src/main/java/com/linecorp/centraldogma/internal/api/v1/AccessToken.java

+
+    @JsonProperty("expires_in")
+    public long expiresIn() {
+        return expiresIn;


I wonder if we should use expires_at so that a client knows when exactly a token will be expired. Or is this a part of OAUTH2 specification?

Yes, it is a part of the spec. :-)

trustin · 2018-03-08T05:26:38Z

...c/main/java/com/linecorp/centraldogma/server/internal/admin/authentication/LoginService.java

+            return new UsernamePasswordToken(username, password);
+        }
+
+        return throwResponseException("invalid_request", "request was missing the username or password.");


request must contain username and password.?

trustin · 2018-03-08T05:30:24Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/HttpApiUtil.java

+     * {@link JsonNode}.
+     */
+    public static HttpResponseException newHttpResponseException(HttpStatus status, JsonNode node) {
+        return HttpResponseException.of(newResponseWithJson(status, node));


Perhaps we should provide a way to specify an HttpStatus with a response object in Armeria?

I was think about adding methods whose signatures are same as HttpResponse.of(...), but I was not sure which methods I needed to add to HttpResponseException, because there are a lot. :-)
To handle above case, I need a method like this.

public static HttpResponseException of(HttpStatus status, MediaType mediaType, byte[] content) { ... } or public static HttpResponseException of(HttpHeaders headers, byte[] content) { ... }

Yeah we should add those methods so that a user can send an error response conveniently. However, I guess a user will find it weird to throw an exception when sending a successful response.

That's right. Do we need to check the status here if we add those methods?

I guess not. You know, some peculiar API uses 200 OK for error responses ;-)

Yeah, we talked about it in this morning. lol

trustin · 2018-03-08T05:32:17Z

.../java/com/linecorp/centraldogma/server/internal/admin/authentication/LoginAndLogoutTest.java

+                               HttpHeaderNames.AUTHORIZATION, "basic " + encoder.encodeToString(
+                                       (USERNAME + ':' + PASSWORD).getBytes(StandardCharsets.US_ASCII)),
+                               HttpHeaderNames.CONTENT_TYPE, MediaType.FORM_DATA.toString()),
+                "grant_type=client_credentials",


Shouldn't this be password?

From the Golang oauth2 client, I thought it was "grant_type=client_credentials".
But I checked again the spec, and it actually does not require the grant_type.
So I will remove it :-)

I thought I deleted this review comment but seems like I forgot. :-$

codecov-io · 2018-03-09T09:38:20Z

Codecov Report

Merging #141 into master will decrease coverage by 0.01%.
The diff coverage is 87.09%.

@@            Coverage Diff            @@
##           master    #141      +/-   ##
=========================================
- Coverage   63.51%   63.5%   -0.02%     
=========================================
  Files         273     273              
  Lines        9451    9474      +23     
  Branches     1065    1063       -2     
=========================================
+ Hits         6003    6016      +13     
- Misses       2825    2834       +9     
- Partials      623     624       +1

Impacted Files	Coverage Δ
...raldogma/server/internal/api/ContentServiceV1.java	`91.08% <100%> (-1.7%)`	⬇️
...centraldogma/server/internal/api/DtoConverter.java	`96.15% <100%> (+4.84%)`	⬆️
...corp/centraldogma/internal/api/v1/AccessToken.java	`100% <100%> (ø)`
...inecorp/centraldogma/internal/api/v1/EntryDto.java	`66.66% <100%> (-3.04%)`	⬇️
...com/linecorp/centraldogma/server/CentralDogma.java	`72.13% <100%> (-0.63%)`	⬇️
...er/internal/admin/authentication/LoginService.java	`73.01% <68.75%> (-4.26%)`	⬇️
.../centraldogma/server/internal/api/HttpApiUtil.java	`78.57% <83.33%> (-0.6%)`	⬇️
...necorp/centraldogma/internal/api/v1/CommitDto.java	`63.15% <88.88%> (-1.55%)`	⬇️
...ternal/api/converter/HttpApiResponseConverter.java	`63.63% <0%> (-9.1%)`	⬇️
... and 4 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 bc249fc...0f5485d. Read the comment docs.

trustin · 2018-03-12T04:56:07Z

client/go/dogma/dogma.go

+	PathPrefixV0      = "api/v0/"
+	DefaultBaseURL    = DefaultScheme + "://" + DefaultHostName + ":36462/"
+
+	defaultPort         = 36462


I actually meant making all constants private, except DefaultPort. :-)

Oops~~ let me fix this~

hyangtack · 2018-03-13T05:21:12Z

client/go/dogma/dogma.go

+		} else {
+			buf = new(bytes.Buffer)
+			enc := json.NewEncoder(buf)
+			//enc.SetEscapeHTML(true)


nit: Is this necessary?

It's necessary. Forgot to bring it back. Thank you!

hyangtack · 2018-03-13T06:31:01Z

...c/main/java/com/linecorp/centraldogma/server/internal/admin/authentication/LoginService.java

@@ -60,14 +71,13 @@ public LoginService(CentralDogmaSecurityManager securityManager, CommandExecutor
    protected HttpResponse doPost(ServiceRequestContext ctx, HttpRequest req) throws Exception {
        final CompletableFuture<HttpResponse> future = new CompletableFuture<>();
        req.aggregate().thenAccept(aMsg -> {


Now we can use AggregatedHttpMessage on the parameter list. So, how about changing HttpRequest to AggregatedHttpMessage and removing the aggregation from here?

Fixed! Thank you~!

Oops I realized that this class inherits the AbstractHttpService and the method overrides, so I can not change :-)

minwoox · 2018-03-13T11:49:42Z

As described in #59, we are going to remove Thrift :)

huydx · 2018-03-13T14:17:15Z

client/go/dogma/content_service.go

+}
+
+func (con *contentService) getFiles(ctx context.Context,
+	projectName, repoName, revision, pathPattern string) ([]*Entry, *http.Response, error) {


is there any chance that user would care about http.Response? Adding response will make the API couple to http package.

That's a good question~! This is the one that I barely made a decision.

Some go client libraries such as go-github that I referred, returns the *http.Response.
I was thinking that a user may want to take a look at the response when something unexpected happens.
And I made the constructor for client which takes the *http.Client as an argument. So I though t is was OK. If the user does not want it, the user can just ignore it with _.
But I think I can take that out as well. :-)
Do you think it is too tight with the http package?

huydx

Leave few nits comments.
One more question: as a user I want something like Watcher in java client, is it possible without Thrift or we need to make some REST API for polling?

huydx · 2018-03-13T14:21:17Z

client/go/dogma/content_service.go

+
+	if len(revision) != 0 {
+		u += "?revision=" + revision
+	}


how about use url#Values pacakge to format

In order to do that, parse the string as url.URL and change it back to string when it invokes newRequest().
That's why I was reluctant to use the package, but probably it's better use it. :-)

Oops, I totally misunderstood the API. Thank you for letting me know that.

huydx · 2018-03-13T14:25:43Z

client/go/dogma/content_service.go

+	u := fmt.Sprintf("%vprojects/%v/repos/%v/commits/%v", defaultPathPrefix, projectName, repoName, from)
+
+	urlQuery := ""
+	if len(pathPattern) != 0 {


ditto (using url#Values instead)

huydx · 2018-03-13T14:34:49Z

client/go/dogma/content_service.go

+
+type Push struct {
+	CommitMessage *CommitMessage `json:"commitMessage"`
+	Changes       []Change       `json:"changes"`


how about use []*Change instead for uniformity

That's a good idea.
Is there any convenient way to convert the []Change to []*Change?

I'm afraird not :(

I just changed the method parameter to take []*Change.
I realized that I don't have to take the argument as varargs because I need at least one Change.
Thank you~!

huydx · 2018-03-13T14:41:32Z

client/go/dogma/dogma.go

+
+// CreateProject creates a project.
+func (c *Client) CreateProject(ctx context.Context, name string) (*Project, *http.Response, error) {
+	return c.project.create(ctx, name)


Not sure what is better, but compare between below 2 interfaces, I prefer former one. HDYT?

// expose service object dogma.Project.Create(...)

vs

// current approach dogma.CreateProject(...)

Either way is fine to me. Just wanted to have same method name as in Java client library :-)

same method name as in Java client

this sound reasonable, so let's keep compability for emthod naming rule

huydx · 2018-03-13T14:52:48Z

client/go/dogma/dogma.go

+	import "golang.org/x/oauth2/clientcredentials"
+
+	func Client() {
+		config := clientcredentials.Config{ClientID: clientID,


we support oauth only and not basic auth?

This authClient actually uses basic auth when it requests a token. But I think I need to change the example because this can confuse :-)

minwoox · 2018-03-14T03:46:37Z

One more question: as a user I want something like Watcher in java client, is it possible without Thrift or we need to make some REST API for polling?

I am planning to make the Watcher in different PR. I'm still trying to figure out which is the best way to implement it. :-)

Motivation: If we provide Go client library, more people can adapt Central Dogma. Modifications: - Add Go client library Result: - More customers Todos: - Repackage the original sources - Adapt CLI using this library - Adapt OAuth2 client - Add the watcher

minwoox · 2018-03-15T09:27:05Z

@huydx , Fixed the issues, please take a look.

huydx · 2018-03-15T10:35:29Z

client/go/dogma/content_service.go

+	// QueryType can be "identity" or "json_path". "identity" is used to retrieve the content as it is.
+	// "json_path" applies a series of JSON path to the content.
+	// See https://github.com/json-path/JsonPath/blob/master/README.md
+	QueryType   string


sorry I missed that at first round, do you think string is better than iota?
Using iota may need some more boilerplate code to generate equivalent string, but for users it's easier to use because they know that what type of QueryType is supported, and prevent typo as well.
In case you find it using iota will add too much boiler plate, how about define some constant for those strings?

iota should be much better~! Thank you~!

huydx · 2018-03-15T10:37:40Z

client/go/dogma/content_service.go

+// Entry represents an entry in the repository.
+type Entry struct {
+	Path       string      `json:"path"`
+	Type       string      `json:"type"` // can be JSON, TEXT or DIRECTORY


same with above comment about iota

This field is used to store the JSON response. If I use this as iota, I need a custom deserializer to convert the sting to corresponding iota number.
Can I implement the custom deserializer or is it better to use predefined string constants?

As a user, I would prefer iota constant. IMHO it's fine to add deserialize function like func (o *Type) UnmarshalJSON(b []byte) error {}

Got it. I will try the way you suggest. Thang you!

huydx · 2018-03-15T10:38:21Z

client/go/dogma/content_service.go

+	Path string `json:"path"`
+
+	// can be UPSERT_JSON, UPSERT_TEXT, REMOVE, RENAME, APPLY_JSON_PATCH or APPLY_TEXT_PATCH
+	Type    string      `json:"type"`


same with above comment about iota

huydx · 2018-03-15T10:42:09Z

client/go/dogma/dogma.go

+	return newClientWithHTTPClient(normalizedURL.String(), config.Client(context.Background()))
+}
+
+// newClientWithHTTPClient returns a Central Dogma client withe the specified baseURL and client.


huydx · 2018-03-15T10:46:29Z

client/go/dogma/dogma.go

+}
+
+// NewClient returns a Central Dogma client with the specified baseURL, clientID and clientSecret.
+func NewClient(baseURL string, clientID, clientSecret string) (*Client, error) {


how about 2 separate constructors like

NewClient(baseUrl string) *Client, error { } NewSecureClient(baseURL string, clientID, clientSecret string) *Client, error { }

And in inner implement we will check if clientID and clientSecret is empty, we will use secure API and opposite?
Not sure about the spec of CD server right now.

I think having just one constructor is fine for now so that the client does not have to handle the authentication by itself. Let's add it when we think it really needs :-)

huydx · 2018-03-15T10:48:58Z

@minwoox added second round comments

minwoox · 2018-03-16T06:45:25Z

@huydx Please take a look again~. I was trying to use some library like enumer to convert the iota values, but it seems like I coudln't not have different names for the enum. For example, we use UPSERT_JSON in the Central Dogma server, but
If I name the values as golang style like UpsertJSON, I cannot convert the value using that library.
So, I decide to just declare maps and arrays to convert that.

huydx · 2018-03-16T10:05:39Z

LGTM! Should I merge it @minwoox

minwoox · 2018-03-16T10:10:04Z

Thanks @huydx. Let's wait a little bit in case @trustin, @hyangtack want to leave a comment.

trustin

Still LGTM

minwoox · 2018-03-19T06:10:35Z

I misunderstood the Oauth2 spec. I removed the client credentials part. :-)

hyangtack · 2018-03-19T08:22:04Z

client/go/dogma/dogma.go

-// NewClient returns a Central Dogma client with the specified baseURL, clientID and clientSecret.
-func NewClient(baseURL string, clientID, clientSecret string) (*Client, error) {
+// NewClient returns a Central Dogma client with the specified baseURL, username and password.
+func NewClient(baseURL string, username, password string) (*Client, error) {


nit: Just curious. username -> username string? Is the type unnecessary?

Oops~! Thank you for pointing it out. (baseURL, username, password string) is correct because all of the three parameters are string, I can just put one string at the end. (does not have to put type for every parameter, if they are same)

minwoox · 2018-03-21T05:31:17Z

Thanks reviews~!

minwoox requested review from trustin, huydx and hyangtack March 5, 2018 09:50

minwoox force-pushed the go_library branch 3 times, most recently from a2f9202 to 6afa71c Compare March 7, 2018 10:32

trustin added the new feature label Mar 8, 2018

trustin added this to the 0.22.0 milestone Mar 8, 2018

trustin requested changes Mar 8, 2018

View reviewed changes

minwoox force-pushed the go_library branch 5 times, most recently from 47c9d25 to 8c4065f Compare March 8, 2018 11:06

trustin modified the milestones: 0.22.0, 0.23.0 Mar 9, 2018

minwoox force-pushed the go_library branch 3 times, most recently from ffb69f3 to 1903c0b Compare March 9, 2018 09:24

trustin reviewed Mar 12, 2018

View reviewed changes

minwoox force-pushed the go_library branch from 1903c0b to dc158d5 Compare March 12, 2018 05:35

trustin mentioned this pull request Mar 12, 2018

Provide annotated services a way to return a successful response with alternative status code line/armeria#1081

Closed

minwoox force-pushed the go_library branch 3 times, most recently from e178d13 to 9781a8e Compare March 12, 2018 12:49

trustin approved these changes Mar 12, 2018

View reviewed changes

hyangtack reviewed Mar 13, 2018

View reviewed changes

hyangtack approved these changes Mar 13, 2018

View reviewed changes

minwoox force-pushed the go_library branch from 34788f0 to 29f5aff Compare March 13, 2018 09:56

huydx reviewed Mar 13, 2018

View reviewed changes

huydx approved these changes Mar 13, 2018

View reviewed changes

minwoox and others added 5 commits March 15, 2018 16:57

Provide Go client library

e414f65

Motivation: If we provide Go client library, more people can adapt Central Dogma. Modifications: - Add Go client library Result: - More customers Todos: - Repackage the original sources - Adapt CLI using this library - Adapt OAuth2 client - Add the watcher

Modify LoginService

a606ba8

Add package documentation

8cd7609

Modify auth.session.service.js

456eb83

Address comments

e4fcfb2

minwoox force-pushed the go_library branch from 29f5aff to e4fcfb2 Compare March 15, 2018 09:25

huydx reviewed Mar 15, 2018

View reviewed changes

Address comments second

b864448

minwoox force-pushed the go_library branch from a1f7bac to b864448 Compare March 16, 2018 06:41

trustin approved these changes Mar 19, 2018

View reviewed changes

hyangtack reviewed Mar 19, 2018

View reviewed changes

Change back to use username and password

0f5485d

minwoox force-pushed the go_library branch from a0ed7fa to 0f5485d Compare March 19, 2018 09:37

minwoox merged commit df35d84 into line:master Mar 21, 2018

minwoox deleted the go_library branch March 21, 2018 05:31


		private final String refreshToken;

		//TODO(minwoox) add scope when it needs

Provide Go client library #141

Provide Go client library #141

Conversation

minwoox commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Mar 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 9, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Mar 13, 2018

Choose a reason for hiding this comment

minwoox Mar 14, 2018 • edited

Choose a reason for hiding this comment

huydx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huydx Mar 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Mar 14, 2018

minwoox commented Mar 15, 2018

huydx Mar 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huydx commented Mar 15, 2018

minwoox commented Mar 16, 2018

huydx commented Mar 16, 2018

minwoox commented Mar 16, 2018

trustin left a comment

Choose a reason for hiding this comment

minwoox Mar 8, 2018 •

edited

minwoox Mar 8, 2018 •

edited

minwoox Mar 9, 2018 •

edited

codecov-io commented Mar 9, 2018 •

edited

minwoox Mar 14, 2018 •

edited

huydx Mar 15, 2018 •

edited

huydx Mar 15, 2018 •

edited