Sdk 2246 go retrieve session #282

mehmet-yoti · 2023-08-08T08:40:58Z

No description provided.

fofiuiancu

Not covered by this PR but should have been covered by the previous PR:
The ShareSessionRequest should have the optional fields as pointers:
(eg policy is mandatory in Share V2 so it shouldn't be a pointer)

Please see the optional ones here:
https://lampkicking.atlassian.net/wiki/spaces/DAS/pages/3900801110/Share+V2+Payload+documentation

_examples/idv/routes.go

digital_identity_client.go

digitalidentity/service.go

digitalidentity/service_test.go

fofiuiancu · 2023-08-30T17:01:54Z

Not covered by this PR but should have been covered by the previous PR: The ShareSessionRequest should have the optional fields as pointers: (eg policy is mandatory in Share V2 so it shouldn't be a pointer)

Please see the optional ones here: https://lampkicking.atlassian.net/wiki/spaces/DAS/pages/3900801110/Share+V2+Payload+documentation

this should also be addressed

digital_identity_client.go

digital_identity_client_test.go

digitalidentity/service.go

_examples/idv/models.sessionspec.go

digitalidentity/share_session.go

digitalidentity/share_session_builder.go

digitalidentity/service.go

digitalidentity/service_test.go

Co-authored-by: Iancu Fofiu <[email protected]>

digital_identity_client.go

klaidas · 2023-09-05T17:11:57Z

digital_identity_client_test.go

+		Key: key,
+	}
+
+	_, err := client.GetSession("SOME ID")


let's add a success test case for this too

digitalidentity/service.go

klaidas · 2023-09-05T17:29:44Z

digitalidentity/service.go

+		return share, err
+	}
+
+	response, err := requests.Execute(httpClient, request, ShareSessionHTTPErrorMessages, yotierror.DefaultHTTPErrorMessages)


i think we need to update this requests package to support the error codes connect send back, some of these are really useful and we should differentiate between them and give the RP an exported error var they can switch on and handle.

e.g. from looking at this 404 UNKNOWN_SESSION is invalid session id which would be useful for a RP to know when integrating with sdk, as opposed to the ShareSessionHTTPErrorMessages where 404 is mapped to Application was not found?

@mehmet-yoti are you still working on this?

Yes I am doing some changes on that.

i don't see the changes for this from the newest commits @mehmet-yoti - feel free to ping me on slack if you wanna chat about it abit quicker

digitalidentity/share_session.go

digitalidentity/share_session_builder.go

Co-authored-by: Klaidas Urbanavicius <[email protected]>

fofiuiancu · 2023-10-06T15:58:56Z

Not covered by this PR but should have been covered by the previous PR: The ShareSessionRequest should have the optional fields as pointers: (eg policy is mandatory in Share V2 so it shouldn't be a pointer)
Please see the optional ones here: https://lampkicking.atlassian.net/wiki/spaces/DAS/pages/3900801110/Share+V2+Payload+documentation

this should also be addressed

ShareSessionNotification should also be optional and the Builder should take care of initialising the optionals with defaults where is the case (I a not sure why the extensions is populated in the Builder with an empty array, but haven't investigated it in depth)

It's not clear from the documentation is we should initialise the optionals or if Connect does that if they are not set.

https://lampkicking.atlassian.net/wiki/spaces/DAS/pages/3900801110/Share+V2+Payload+documentation

_examples/idv/handlers.session.go

yotierror/response.go

digital_identity_client_test.go

fofiuiancu · 2023-10-06T16:01:59Z

digitalidentity/service.go

@@ -28,14 +28,16 @@ func CreateShareSession(httpClient requests.HttpClient, shareSessionRequest *Sha
 		HTTPMethod: http.MethodPost,
 		BaseURL:    apiUrl,
 		Endpoint:   endpoint,
-		Headers:    requests.AuthHeader(clientSdkId),
+		Headers:    requests.AuthHeader(clientSdkId, &key.PublicKey),


it seems that we don't need the public key here; do we need the clientSDKId as we're also adding it a few rows later as a Param?

As read from documentation backend needs this header.

It seems that we don't need it the Params (but we need it in the Headers)

digitalidentity/share_session_builder.go

fofiuiancu · 2023-10-06T16:09:43Z

digitalidentity/share_session_notification_builder_test.go

@@ -18,7 +18,7 @@ func ExampleShareSessionNotificationBuilder() {
 	}

 	fmt.Println(string(data))
-	// Output: {"url":"","method":"","verifyTls":null,"headers":null}
+	// Output: {"url":""}


maybe here is the point the defaults should have been set?

requests/signed_message.go

fofiuiancu · 2023-10-06T16:11:17Z

sonar-project.properties

@@ -11,4 +11,4 @@ sonar.go.coverage.reportPaths = coverage.out
 sonar.go.tests.reportPaths = sonar-report.json
 sonar.tests = .
 sonar.test.inclusions = **/*_test.go
-sonar.coverage.exclusions = test/**/*,_examples/**/*
+sonar.coverage.exclusions = test/**/*,_examples/**/*,src/digitalidentity/**


I am not sure this is the right approach. How did the other SDKs solve it?

We should only exclude digitalidentity only from code duplication checks (so use sonar.cpd.exclusions)

yotierror/response.go

fofiuiancu

see comments

…e the new error format

klaidas · 2023-10-12T22:52:07Z

digital_identity_client_test.go

+
+func TestDigitalIDClient_GetSession(t *testing.T) {
+	key, err := os.ReadFile("./test/test-key.pem")
+	if err != nil {


looks like the rest of the code in this repo uses assert package so prob best to be consistent

klaidas · 2023-10-12T22:56:49Z

digitalidentity/yotierror/response.go

+	defaultUnknownErrorCodeConst    = "UNKNOWN_ERROR"
+	defaultUnknownErrorMessageConst = "unknown HTTP error"


why's the var named ..Const instead of just being consts?

Just kept the convention from the V1 SDK

that's not a good convention to follow 😆

digitalidentity/yotierror/response.go

yotierror/response.go

digitalidentity/share_session_notification_builder.go

digitalidentity/share_session_builder.go

digitalidentity/service_test.go

klaidas · 2023-10-12T23:23:19Z

digitalidentity/yotierror/signed_requests.go

@@ -0,0 +1,8 @@
+package yotierror
+
+const (


these aren't used

True, but this is a package meant to be used by integrators so I;d like to keep them in if we believe they can add value as consts to compare against.

well they would add value if they were used, if they're not used why would they need to compare against them?

klaidas · 2023-10-12T23:28:11Z

digitalidentity/requests/signed_message.go

@@ -0,0 +1,222 @@
+package requests


what was the reason for copying this entire package? it's better to have 1 copy of common code so we can maintain it easier and it won't bite us in the ass if we make a fix to one but forget the copy.

if the whole reason was because we need it to use the new yotierror response to error parsing, we can just version the Execute function like Execute2 or something (or if the name is important like it will be used directly by integrators then we could just have this function by itself inside the digitalidentity package instead) and have that do the new error handling

Good points, but, as we plan to retire V1 I thought this version is preferable. That's the same reason the choice was taken to duplicate the Policy code. See the notes here: https://lampkicking.atlassian.net/browse/SDK-2230.

fofiuiancu · 2023-10-13T09:02:04Z

_examples/idv/handlers.session.go

@@ -63,6 +63,7 @@ func pageFromSessionSpec(c *gin.Context, sessionSpec *create.SessionSpecificatio
 		return
 	}
 	createSessionResult, err = client.CreateSession(sessionSpec)
+


we can remove this

fofiuiancu

Only some minor things left, for me the main one is "use sonar.cpd.exclusions"

Co-authored-by: Klaidas Urbanavicius <[email protected]>

mehmet-yoti added 3 commits July 20, 2023 14:50

SDK-2240:fixed multiple error vars

0a7667c

SDK-2246:go retrieve session

8825126

SDK-2246:go retrieve session update

82b3fc9

mehmet-yoti requested a review from fofiuiancu August 8, 2023 08:42

fofiuiancu requested changes Aug 21, 2023

View reviewed changes

mehmet-yoti added 3 commits August 22, 2023 20:24

SDK-2246:Resolved Problems

b326008

SDK-2246:Resolved Problems

c30c85b

SDK-2246:update review changes

d0f8123

mehmet-yoti requested a review from fofiuiancu August 29, 2023 10:38

SDK-2246:removed buildDigitalIdentitySessionSpec

f0be59e

fofiuiancu requested changes Aug 30, 2023

View reviewed changes

mehmet-yoti requested a review from klaidas August 31, 2023 15:29

mehmet-yoti and others added 4 commits August 31, 2023 16:30

Update digitalidentity/service_test.go

f6de789

Co-authored-by: Iancu Fofiu <[email protected]>

Update digitalidentity/share_session.go

dc8aeb7

Co-authored-by: Iancu Fofiu <[email protected]>

Update digital_identity_client.go

3d92450

Co-authored-by: Iancu Fofiu <[email protected]>

Update digital_identity_client.go

391b925

Co-authored-by: Iancu Fofiu <[email protected]>

klaidas reviewed Sep 5, 2023

View reviewed changes

mehmet-yoti and others added 12 commits September 6, 2023 12:18

SDK-2246 refactor needed changes

49a758c

SDK-2246 refactor needed changes

bdda8e4

Update digitalidentity/share_session.go

d011b02

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digitalidentity/share_session.go

6f5d220

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digitalidentity/service.go

8b2a670

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digitalidentity/service.go

ade2267

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digital_identity_client.go

9fe3f6c

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digital_identity_client.go

8c88429

Co-authored-by: Klaidas Urbanavicius <[email protected]>

SDK-2246 refactor needed changes

4a9ff75

SDK-2246 update tests and httpstatus codes

ca50060

SDK-2246 update tests and httpstatus codes

787a94b

SDK-2246 update tests and httpstatus codes

198f704

mehmet-yoti added 6 commits September 26, 2023 09:35

SDK-2246 Cleared commented lines

565204d

SDK-2246 updated optional parameters

0fa3553

SDK-2246 updated optional parameters

3321f3e

SDK-2246 updated optional parameters

c7d96fc

SDK-2246 removed unnecessary files

c4c3290

SDK-2246 updated tests to match with optional values

004d711

mehmet-yoti requested a review from fofiuiancu September 27, 2023 07:23

mehmet-yoti added 3 commits September 27, 2023 08:44

SDK-2246 add updated test file

edf368c

SDK:2246 update sonar properties

9d43a02

SDK:2246 update sonar properties

d99947b

fofiuiancu reviewed Oct 6, 2023

View reviewed changes

fofiuiancu requested changes Oct 6, 2023

View reviewed changes

Iancu Fofiu and others added 2 commits October 11, 2023 15:19

SDK-2246: added digitalidentity specific request and yotierr to handl…

1c74f09

…e the new error format

SDK-2246:pr updates

aad8d97

klaidas reviewed Oct 12, 2023

View reviewed changes

fofiuiancu reviewed Oct 13, 2023

View reviewed changes

mehmet-yoti and others added 4 commits October 13, 2023 11:50

Update digitalidentity/share_session_builder.go

75fe18e

Co-authored-by: Klaidas Urbanavicius <[email protected]>

Update digitalidentity/yotierror/response.go

6566ede

Co-authored-by: Klaidas Urbanavicius <[email protected]>

SDK-2246:updates for pr checks

fba5a14

SDK-2246:updates for pr checks

0df76b0

mehmet-yoti requested review from fofiuiancu and klaidas October 13, 2023 11:35

klaidas approved these changes Oct 13, 2023

View reviewed changes

fofiuiancu approved these changes Oct 13, 2023

View reviewed changes

mehmet-yoti changed the base branch from SDK-2240-sharev2-go-create-session to SDK-2230-Expose-share-v2-api October 15, 2023 14:05

mehmet-yoti changed the base branch from SDK-2230-Expose-share-v2-api to SDK-2240-go-create-session October 15, 2023 14:06

mehmet-yoti changed the base branch from SDK-2240-go-create-session to SDK-2230-Expose-share-v2-api October 15, 2023 14:10

mehmet-yoti merged commit caf4d1a into SDK-2230-Expose-share-v2-api Oct 15, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdk 2246 go retrieve session #282

Sdk 2246 go retrieve session #282

mehmet-yoti commented Aug 8, 2023

fofiuiancu left a comment

fofiuiancu commented Aug 30, 2023

klaidas Sep 5, 2023

klaidas Sep 5, 2023

klaidas Sep 7, 2023

mehmet-yoti Sep 7, 2023

klaidas Sep 19, 2023

fofiuiancu commented Oct 6, 2023 •

edited

Loading

fofiuiancu Oct 6, 2023

mehmet-yoti Oct 12, 2023

fofiuiancu Oct 13, 2023

fofiuiancu Oct 6, 2023

fofiuiancu Oct 6, 2023

fofiuiancu Oct 13, 2023

fofiuiancu left a comment

klaidas Oct 12, 2023

klaidas Oct 12, 2023

fofiuiancu Oct 13, 2023

klaidas Oct 13, 2023

klaidas Oct 12, 2023

fofiuiancu Oct 13, 2023

klaidas Oct 13, 2023

klaidas Oct 12, 2023

fofiuiancu Oct 13, 2023

fofiuiancu Oct 13, 2023

fofiuiancu left a comment

		defaultUnknownErrorCodeConst = "UNKNOWN_ERROR"
		defaultUnknownErrorMessageConst = "unknown HTTP error"

Sdk 2246 go retrieve session #282

Sdk 2246 go retrieve session #282

Conversation

mehmet-yoti commented Aug 8, 2023

fofiuiancu left a comment

Choose a reason for hiding this comment

fofiuiancu commented Aug 30, 2023

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

fofiuiancu commented Oct 6, 2023 • 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

fofiuiancu 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

fofiuiancu left a comment

Choose a reason for hiding this comment

fofiuiancu commented Oct 6, 2023 •

edited

Loading