upgrade ver 0.1.0 (beta release) #35

darai0512 · 2022-09-01T07:07:26Z

対応箇所と修正方針

nullを表現する
- pay.jpのAPIレスポンスはnullを表現として利用する（リクエストでは引数指定しないことが該当）
- 言語としてゼロ値埋めという機能があるため、nullを表現できていないレスポンスがあったり、リクエストパラメータがゼロ値で指定したかのように送られたりする
- これを回避する方法として、各構造体の型を全てポインタ型に統一したい（競合他社SDKのインターフェースと揃うので移行時のメリットにもなる）
- ただし後方互換性を優先した
  - ex, 同じパラメータを表す別名フィールドを用意（XXXがstring型ならRawXXXという*string型を用意）
リスト検索でlimit/offset/since/until以外使えない状態を修正
- 現状の.List().Limit().Do()のようなチェーンでの使用法をdeprecatedとする
  - ただし後方互換性を優先して残しておく
  - 残しておくが、メンテはしない＝引数が増えてもこちらには足さない
- .All(params)を用意し、paramsにlimitやsubscription_idなどそれぞれで必要な引数を渡してもらう
  - チェーンより実装が楽になるので、メンテコストが下がる
  - 本当は他社SDKとインターフェースを揃えるために.List(params)にしたかったが後方互換性のため断念（上のdeprecatedが完了したら.List(params)も用意したい）
APIの細かな説明・引数バリデーションを削除
- API documentに集約する（go欄を新設する）
- 引数バリデーションは他SDKではしておらず、APIレスポンスにて十分伝わるため
一部のupdate時にmetadataが変更できない問題を対応
- subscription
- plan
SubscriptionResponse構造体がもつUpdate/Pause/Resume/Cancel/Deleteが動作してなかった
- closeしても良かったがPlan側は動いたりしたので直した
- 直し方：トップレベルのSubscriptionService.Update等と同じロジックのコピぺだったので全てそちらを参照するように
payjperror.goのparseResponseErrorを修正
- errorResponseでJSON.Unmarshalすべきなのに一階層下で行なっていた
- delete系APIの戻り値がエラーでもnilを返してしまっていた
- update系でなぜか使われているが、各リソース毎のUnmarshal内で同じことをしているので、経由しないよう修正
サポートバージョンの変更
- LTS: https://endoflife.date/go
- golintの1.12以下のサポートが終わっている。staticcheckなど他のリンターも同様
- 1.12以下はcompile onlyとする
- lintは各バージョンではなく最新版のみ。testは1.13以降
float to stringは利用者側で行う（どういうオプションか不明なため。そもそもfloat引数はなく、それ系はstringで渡すようリファレンスで促している）。元の挙動通りpanicさせている（エラーメッセージ分わかりやすくなった）

他細かい箇所

SubscriptionStatus.String()を用意
- SubscriptionResponse.Statusは数値
- SubscriptionStatus.status()で日本語化できるが、小文字＝プライベート関数のためSDK利用者は使えないし、内部からも使われてなかった
- 外から使えるようにした。ただしこの関数名だとfmt.Print()した際に自動で数値型ではなく文字列型となる。後方互換性を意識するなら関数名を変えても良い

後方互換性のない箇所

いずれもニーズ・使用例が極端に低いであろう箇所のみ

EventType廃止
- 単独で使うニーズはないと思われる（そもそもenumリストが更新されてなかった）
subscriptionでcustomer指定時の戻り値のリストのurlパラメータの値が変わる
- subscriptionのList検索でcustomerIDを指定する方法がこれまでなかった
- customerのsubscription list検索では存在した
- 万が一このAPIが使われていたとしてもこのパラメータを利用しているとは思えないので無視
SubscriptionStatusをfmt.Print()した際に数値型ではなく文字列型となる
- 数値が欲しいわけではないはず

darai0512 · 2022-09-14T06:11:55Z

v1/testhelpers.go

+		b, _ := ioutil.ReadAll(req.Body)
+		s := string(b)
+		t.Body = &s
+	}


util.goのテストではなく、単体テストのヘルパーファイルなため、ファイル名を変更。
このヘルパー自体はgithub.com/jarcoal/httpmockを使った方がモダンでリッチなことができるが、現状の単体テストに限れば上の修正で十分なため、外部ライブラリの使用は一旦見送る。

darai0512 · 2022-09-14T06:13:14Z

v1/util.go

-	}
-	defer resp.Body.Close()
-	return ioutil.ReadAll(resp.Body)
-}


client.goに移した。
意味はないのだが、この関数がこのutilが扱ってる内容の中で浮いていたため。

darai0512 · 2022-09-29T12:45:25Z

v1/plan.go

@@ -42,32 +40,21 @@ type Plan struct {
 //
 // また、支払いの実行日を指定すると、支払い日の固定されたプランを生成することができます。
 func (p PlanService) Create(plan Plan) (*PlanResponse, error) {
-	var errors []string
-	if plan.Amount < 50 || plan.Amount > 9999999 {
-		errors = append(errors, fmt.Sprintf("Amount should be between 50 and 9,999,999, but %d.", plan.Amount))


本題と関係ない修正。
基本的にSDKでバリデーションは、日付など普遍的なもの以外はしないよう改修。
リクエスト自体はさせて、サーバーからのエラーメッセージに任せる。

加盟店によっては50円未満決済可能であるし、将来金額上限やCurrencyが増える可能性もあるため。

この変更は既存ユーザーに影響しない。

darai0512 · 2022-10-03T06:37:33Z

v1/client.go

+				continue
+			}
+			fieldV := v.Field(i)
+			// if fieldV.Kind() != reflect.Ptr || fieldV.IsZero() { // todo golang >= 1.13


本当は使いたいけれどcompile only supportができなくなる。
https://pkg.go.dev/reflect#Value.IsZero

natsuki-yamanaka · 2022-09-30T06:11:52Z

v1/subscription.go

@@ -25,7 +22,7 @@ const (
 	SubscriptionPaused
 )

-func (s SubscriptionStatus) status() interface{} {
+func (s SubscriptionStatus) String() string {


UnmarshalJSONでも似た判定してるので統一したいですね。

payjp-go/v1/subscription.go

Lines 394 to 403 in 744614f

switch raw.Status {

case "active":

s.Status = SubscriptionActive

case "trial":

s.Status = SubscriptionTrial

case "canceled":

s.Status = SubscriptionCanceled

case "paused":

s.Status = SubscriptionPaused

}

軽微なので無視でいいんですが

natsuki-yamanaka · 2022-10-04T01:10:27Z

.github/workflows/build-test.yml

      - name: Execute Test
        run: go test -v ./v1
+      - name: Compile
+        run: go build ./...


これの意味がよく分からないんですが… go build ./v1/*.go ではないんですか

goにおけるワイルドカードのようです。カレントディレクトリ以下を再帰します。
v1/examplesなども含んで欲しいのでこちらにしています。

natsuki-yamanaka · 2022-10-04T04:32:14Z

v1/testhelpers.go

@@ -6,8 +6,8 @@ import (
 	"net/http"
 )

-func NewMockClient(status int, response []byte) (*http.Client, *MockTransport) {
-	transport := &MockTransport{
+func newMockClient(status int, response []byte) (*http.Client, *mockTransport) {


これprivate化しちゃって大丈夫ですかね？
これ使ってテスト書いちゃってるところとかないかな

それならテストこけそう。外の人って意味だと、元々公開前提ではないはずだし考慮しない（後方互換性ない）で良さそう

natsuki-yamanaka · 2022-10-04T06:14:19Z

v1/client_test.go

-	if service.APIBase() != "https://api.pay.jp/v2" {
-		t.Errorf(`ApiBase should be "https://api.pay.jp/v2", but "%s"`, service.APIBase())
+func TestRequests(t *testing.T) {
+	mock, transport := newMockClient(400, errorJSON)


v1/payjperror_test.goのerrorJSONに依存してるのがびっくりしますね。
errorJSONはtesthelpersに移動して良さそう

natsuki-yamanaka · 2022-10-04T09:22:25Z

v1/client.go

-	request.Header.Add("Authorization", s.apiKey)
-
-	_, err = parseResponseError(s.Client.Do(request))
+	_, err = parseError(body)


parseErrorがここでしか使われないならここに中身書いてしまったらどうでしょう。
parseErrorという名前からは汎用的なエラーレスポンスのパースに使われそうで本来各リソースのUnmarshalJSON内でエラーを取り出す処理等にも使われるべきに思えますが、そこまでしないなら逆に違和感がありました。
同じロジック消してparseErrorで共通化するのがより良いとは思いますが、修正をこれ以上大きくするのもまぁためらわれるので…

汎用化done

natsuki-yamanaka · 2022-10-04T09:36:55Z

v1/examples/subscription/main.go

@@ -3,38 +3,151 @@ package main
 import (
 	"fmt"
 	"github.com/payjp/payjp-go/v1"
+	"time"
+
+	//"time"


要らなそう

darai0512 · 2023-02-18T10:41:20Z

v1/plan.go

 // Update はプラン情報を更新します。
-func (p PlanService) Update(id, name string) (*PlanResponse, error) {
-	body, err := p.update(id, name)
+func (p PlanService) Update(id string, plan Plan) (*PlanResponse, error) {


ここは後方互換性のない変更。一応利用状況を確認する？

darai0512 · 2024-02-06T09:21:10Z

v1/customer_test.go

+			Until: Int(1455500895),
+		},
+	}
+	params.Offset = Int(0)


paramsの初期化はListParamsと一段深くなる。けれど初期化後はListParamsというクッションはなくなる。go struct embeddingのルール。

どちらで指定してもらっても構わない。
（一応、他社SDKに揃えるためにこうしたが、そもそもListParamsはいらないかも）

natsuki-yamanaka

バージョンに関してのところを特に相談したい

natsuki-yamanaka · 2024-02-06T07:07:27Z

v1/client.go

 	if err != nil {
 		return nil, err
 	}
 	request.Header.Add("Authorization", s.apiKey)
+	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")


sdkのリリースバージョンを定義したい…

Suggested change

request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

request.Header.Add("User-Agent", "payjp-go-http-client/v1.0.0")

request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go(sdk-version:1.0.0,go-version:"+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

上記にして今回リリースしたらv1.0.0のtagきってリリース作るのはどうすかね。
このPRもただの修正PRではなくタイトルをv1.0.0とかにしていいと思う

done

User-Agentは一旦上書きしないで（これまでと同じ検索ができるようにして）おきます。

natsuki-yamanaka · 2024-02-08T09:24:44Z

v1/client.go

-func (s Service) queryListAll(resourcePath string, limit, offset, since, until, sinceSheduledDate, untilSheduledDate int, callbacks ...func(*url.Values) bool) ([]byte, error) {
-	if limit < 0 || limit > 100 {
-		return nil, fmt.Errorf("method Limit() should be between 1 and 100, but %d", limit)
+func (s Service) makeEncoder(v reflect.Value, values url.Values) {


何の関数か説明を追記して欲しいです。渡されたパラメータをquerystring形式のstringに変換してる？

natsuki-yamanaka · 2024-02-08T09:25:00Z

v1/client.go

-		if callback(&values) {
-			hasParam = true
+// 引数の構造体はメンバが全てポインタ型である必要があります
+func (s Service) getQuery(c interface{}) string {


これも。何の関数か説明を追記して欲しいです

natsuki-yamanaka · 2024-02-08T09:38:54Z

v1/customer.go

+	return c.caller.All(&c.CardListParams)
+}
+
+func (c *CustomerResponse) All(params ...*CardListParams) ([]*CardResponse, bool, error) {


顧客のカードリスト取得はAllCardとかにしたいです

natsuki-yamanaka · 2024-02-09T09:27:19Z

v1/examples/plan/main.go

-		fmt.Println("  LiveMode:", plan.LiveMode)
-		fmt.Println("  Name:", plan.Name)
-		fmt.Println("  TrialDays:", plan.TrialDays)
+	plans, _, _ := payjpService.Plan.List().Limit(1).Do()


Suggested change

plans, _, _ := payjpService.Plan.List().Limit(1).Do()

plans, _, _ := payjpService.Plan.All(&payjp.PlanListParams{

ListParams: payjp.ListParams{

Limit: payjp.Int(1),

},

})

transferのexampleにListを使ってるのが残ってるのでそれも消したい

natsuki-yamanaka · 2024-02-09T09:36:23Z

v1/util.go

@@ -4,8 +4,6 @@ import (
 	"bytes"


このファイルの実装全部client.goに移植でもいい気はする

一旦このまま（見送り）で！

darai0512 · 2024-03-05T07:12:27Z

v1/card.go

 }

 // Delete メソッドは顧客に登録されているカードを削除します
 // Customer情報から得られるカードでしか削除はできません
 func (c *CardResponse) Delete() error {
 	if c.customerID == "" {
-		return errors.New("Token's card doens't support Delete()")


新linterにより。後方互換無くなるが影響極小と判断。

natsuki-yamanaka · 2024-03-05T07:18:17Z

v1/client.go

 	if err != nil {
 		return nil, err
 	}
 	request.Header.Add("Authorization", s.apiKey)
+	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go/"+Version+"("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")


Suggested change

request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go/"+Version+"("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

request.Header.Add("User-Agent", "Go-http-client/payjp-"+Version)

request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go/"+Version+"("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

natsuki-yamanaka · 2024-03-05T07:19:20Z

v1/client_test.go

+	assert.Equal(t, "https://api.pay.jp/v1/test", transport.URL)
+	assert.Equal(t, "POST", transport.Method)
+	assert.Equal(t, "Basic YXBpLWtleTo=", transport.Header.Get("Authorization"))
+	assert.Regexp(t, "^payjp-go/v[0-9.]+\\(go[0-9.]+,os\\:.+,arch\\:.+\\)$", transport.Header.Get("X-Payjp-Client-User-Agent"))


Suggested change

assert.Regexp(t, "^payjp-go/v[0-9.]+\$go[0-9.]+,os\\:.+,arch\\:.+\$$", transport.Header.Get("X-Payjp-Client-User-Agent"))

assert.Regexp(t, "^Go-http-client/payjp-v[0-9.]+$", transport.Header.Get("User-Agent"))

assert.Regexp(t, "^payjp-go/v[0-9.]+\$go[0-9.]+,os\\:.+,arch\\:.+\$$", transport.Header.Get("X-Payjp-Client-User-Agent"))

darai0512 changed the title ~~fix not working 'update metadata'~~ [WIP] fix not working 'update metadata' Sep 8, 2022

darai0512 force-pushed the fix-update-metadata-master branch from a372bdd to 83127b1 Compare September 14, 2022 06:08

darai0512 self-assigned this Sep 14, 2022

darai0512 commented Sep 14, 2022

View reviewed changes

darai0512 force-pushed the fix-update-metadata-master branch 6 times, most recently from 8d3468c to f3ea442 Compare September 14, 2022 11:27

darai0512 requested a review from natsuki-yamanaka September 29, 2022 12:01

darai0512 changed the title ~~[WIP] fix not working 'update metadata'~~ fix not working 'update metadata' Sep 29, 2022

darai0512 commented Sep 29, 2022

View reviewed changes

darai0512 commented Oct 3, 2022

View reviewed changes

natsuki-yamanaka reviewed Oct 4, 2022

View reviewed changes

darai0512 changed the title ~~fix not working 'update metadata'~~ fix not working 'update metadata' + add 3ds params Feb 16, 2023

darai0512 commented Feb 18, 2023

View reviewed changes

darai0512 added 6 commits January 19, 2024 18:04

fix update metadata(sub&plan) & refactor all

11cdbff

fix lint+actions(not working golint under 1.12)

7c22080

reflect.Value.IsZero() needs >1.13, so not compile

83e6fe5

add 3ds params&endpoint

2fdca96

remove expiry_date validation on SDK

77bac3d

add card.three_d_secure_status property

dbd530a

darai0512 force-pushed the fix-update-metadata-master branch 2 times, most recently from 07106a4 to 04b204c Compare January 23, 2024 07:47

darai0512 force-pushed the fix-update-metadata-master branch 4 times, most recently from 80c3329 to 66db653 Compare February 3, 2024 09:35

refactor

43e2537

darai0512 force-pushed the fix-update-metadata-master branch from 66db653 to 43e2537 Compare February 6, 2024 09:19

darai0512 commented Feb 6, 2024

View reviewed changes

natsuki-yamanaka reviewed Feb 9, 2024

View reviewed changes

darai0512 changed the title ~~fix not working 'update metadata' + add 3ds params~~ upgrade ver 0.1.0 Feb 14, 2024

darai0512 changed the base branch from master to develop March 5, 2024 03:46

darai0512 added 4 commits March 5, 2024 13:21

setup lint

f21ef67

add sdk version on X-Payjp-Client-User-Agent

067eb53

fix trivial by review

c3ad0e1

rename Customer.cards.get to AllCard

754e6b5

darai0512 changed the base branch from develop to beta March 5, 2024 05:02

darai0512 changed the title ~~upgrade ver 0.1.0~~ upgrade ver 0.1.0 (beta for standard) Mar 5, 2024

natsuki-yamanaka approved these changes Mar 5, 2024

View reviewed changes

darai0512 commented Mar 5, 2024

View reviewed changes

natsuki-yamanaka reviewed Mar 5, 2024

View reviewed changes

darai0512 force-pushed the fix-update-metadata-master branch 2 times, most recently from 594a170 to c5e58b7 Compare March 6, 2024 08:27

announce by README, rm DataParser, and so on

5c822df

darai0512 force-pushed the fix-update-metadata-master branch from c5e58b7 to 5c822df Compare March 7, 2024 14:56

darai0512 changed the title ~~upgrade ver 0.1.0 (beta for standard)~~ upgrade ver 0.1.0 (beta release) Mar 7, 2024

darai0512 merged commit 9a95e2d into beta Mar 7, 2024
14 checks passed

darai0512 deleted the fix-update-metadata-master branch March 7, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade ver 0.1.0 (beta release) #35

upgrade ver 0.1.0 (beta release) #35

darai0512 commented Sep 1, 2022 •

edited

Loading

darai0512 Sep 14, 2022

darai0512 Sep 14, 2022

darai0512 Sep 29, 2022 •

edited

Loading

darai0512 Oct 3, 2022

natsuki-yamanaka Sep 30, 2022

darai0512 Mar 5, 2024

natsuki-yamanaka Oct 4, 2022

darai0512 Mar 5, 2024

natsuki-yamanaka Oct 4, 2022

darai0512 Feb 18, 2023

natsuki-yamanaka Oct 4, 2022

darai0512 Mar 5, 2024

natsuki-yamanaka Oct 4, 2022

darai0512 Mar 5, 2024

natsuki-yamanaka Oct 4, 2022

darai0512 Feb 18, 2023

darai0512 Feb 6, 2024 •

edited

Loading

natsuki-yamanaka left a comment

natsuki-yamanaka Feb 6, 2024

darai0512 Mar 5, 2024

natsuki-yamanaka Feb 8, 2024

natsuki-yamanaka Feb 8, 2024

natsuki-yamanaka Feb 8, 2024

natsuki-yamanaka Feb 9, 2024

natsuki-yamanaka Feb 9, 2024

darai0512 Mar 5, 2024

darai0512 Mar 5, 2024

natsuki-yamanaka Mar 5, 2024 •

edited

Loading

natsuki-yamanaka Mar 5, 2024 •

edited

Loading

	switch raw.Status {
	case "active":
	s.Status = SubscriptionActive
	case "trial":
	s.Status = SubscriptionTrial
	case "canceled":
	s.Status = SubscriptionCanceled
	case "paused":
	s.Status = SubscriptionPaused
	}

	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")
	request.Header.Add("User-Agent", "payjp-go-http-client/v1.0.0")
	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go(sdk-version:1.0.0,go-version:"+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

-	plans, _, _ := payjpService.Plan.List().Limit(1).Do()
+	plans, _, _ := payjpService.Plan.All(&payjp.PlanListParams{
+		ListParams: payjp.ListParams{
+			Limit: payjp.Int(1),
+		},
+	})

	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go/"+Version+"("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")
	request.Header.Add("User-Agent", "Go-http-client/payjp-"+Version)
	request.Header.Add("X-Payjp-Client-User-Agent", "payjp-go/"+Version+"("+runtime.Version()+",os:"+runtime.GOOS+",arch:"+runtime.GOARCH+")")

	assert.Regexp(t, "^payjp-go/v[0-9.]+\\(go[0-9.]+,os\\:.+,arch\\:.+\\)$", transport.Header.Get("X-Payjp-Client-User-Agent"))
	assert.Regexp(t, "^Go-http-client/payjp-v[0-9.]+$", transport.Header.Get("User-Agent"))
	assert.Regexp(t, "^payjp-go/v[0-9.]+\\(go[0-9.]+,os\\:.+,arch\\:.+\\)$", transport.Header.Get("X-Payjp-Client-User-Agent"))

upgrade ver 0.1.0 (beta release) #35

upgrade ver 0.1.0 (beta release) #35

Conversation

darai0512 commented Sep 1, 2022 • edited Loading

対応箇所と修正方針

後方互換性のない箇所

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darai0512 Sep 29, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darai0512 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

natsuki-yamanaka 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

natsuki-yamanaka Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

natsuki-yamanaka Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

darai0512 commented Sep 1, 2022 •

edited

Loading

darai0512 Sep 29, 2022 •

edited

Loading

darai0512 Feb 6, 2024 •

edited

Loading

natsuki-yamanaka Mar 5, 2024 •

edited

Loading

natsuki-yamanaka Mar 5, 2024 •

edited

Loading