fix: omitempty for VerifiedAt and StateChangedAt #1736

AntiSC2 · 2021-09-07T21:56:41Z

Currently the Python SDK is unusable because the spec doesn't match what Kratos actually returns. OpenAPI expects that Kratos only returns date-time strings for state_changed_at and verified_at. I think the original idea was that if verified_at was null it would be omitted entirely. The problem is that omitempty doesn't work for structs in Go, only pointers to structs. This means that verified_at is returned as null anyways from the API.

This PR changes VerifiedAt and StateChangedAt into pointers so that omitempty works as expected.

Related issue(s)

ory/sdk#95

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

codecov · 2021-09-07T22:38:30Z

Codecov Report

Merging #1736 (adcf8ad) into master (43c3150) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
- Coverage   74.03%   73.98%   -0.05%     
==========================================
  Files         260      260              
  Lines       12696    12701       +5     
==========================================
- Hits         9399     9397       -2     
- Misses       2672     2679       +7     
  Partials      625      625

Impacted Files	Coverage Δ
identity/identity_verification.go	`65.00% <ø> (ø)`
identity/handler.go	`89.15% <100.00%> (+0.26%)`	⬆️
identity/identity.go	`88.57% <100.00%> (+0.10%)`	⬆️
persistence/sql/persister_identity.go	`63.43% <100.00%> (+0.13%)`	⬆️
selfservice/strategy/link/strategy_verification.go	`63.88% <100.00%> (+0.25%)`	⬆️
cmd/courier/watch.go	`60.34% <0.00%> (-12.07%)`	⬇️

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 43c3150...adcf8ad. Read the comment docs.

aeneasr

Thank you! I think a better approach would be to try and return nil isZero is true here:

https://github.com/ory/x/blob/a065f5780b6040793599f5d9fd68a246322b56ab/sqlxx/types.go#L104

If a test is added to ensure it behaves as expected, and it works, it would mean that this is fixed for all fields immediately. I recommend using the "gjson" library in the test.

aeneasr · 2021-09-08T06:49:13Z

identity/identity_verification.go

@@ -68,7 +68,7 @@ type VerifiableAddress struct {
 	//
 	// example: 2014-01-01T23:28:56.782Z
 	// required: false
-	VerifiedAt sqlxx.NullTime `json:"verified_at,omitempty" faker:"-" db:"verified_at"`
+	VerifiedAt sqlxx.NullTime `json:"verified_at" faker:"-" db:"verified_at"`


We could also try making this a pointer instead?

aeneasr · 2021-09-08T06:50:38Z

internal/httpclient/api/openapi.yaml

@@ -2893,6 +2893,7 @@ components:
      format: date-time
      title: NullTime implements sql.NullTime functionality.
      type: string
+      nullable: true


You will need to do this somewhere else as this is autogenerated. In particular, it will probably require a patch along the lines of this one:

kratos/.schema/openapi/patches/schema.yaml

Lines 15 to 17 in 4f05d64

- op: add

path: /components/schemas/uiNodeInputAttributes/properties/value/nullable

value: true

AntiSC2 · 2021-09-12T20:27:31Z

Thank you! I think a better approach would be to try and return nil isZero is true here:

https://github.com/ory/x/blob/a065f5780b6040793599f5d9fd68a246322b56ab/sqlxx/types.go#L104

If a test is added to ensure it behaves as expected, and it works, it would mean that this is fixed for all fields immediately. I recommend using the "gjson" library in the test.

Maybe I'm not understanding it correctly but doesn't the code already return nil there (more specifically, json.Marshal(nil)). As far as I understand the omitempty option only works for things that have a default zero value which structs don't. So we could change VerifiedAt to be a pointer but we also have to do the same change for StateChangedAt.

Would that approach work, changing both fields to pointers and having omitempty on both of them?

aeneasr · 2021-09-14T08:14:00Z

You are right, I thought that we could do this

func (ns NullTime) MarshalJSON() ([]byte, error) {
	t := time.Time(ns)
	if t.IsZero() {
		return nil, nil
	}
	return json.Marshal(&t)
}

but apparently this causes an error in the JSON compiler so it won't work. You are right thus, these fields must indeed be pointers for omitempty to work!

Would that approach work, changing both fields to pointers and having omitempty on both of them?

Yes, that should work and it would be awesome if you could contribute towards this. Please also add a test or two covering this use case so we do not regress this at some point! :)

AntiSC2 · 2021-09-14T17:41:37Z

I think I have manged it now. There are already a lot of existing tests (both in the regular tests and the e2e-tests) checking if the verified_at/state_changed_at field was null so I changed it so they instead check that the field doesn't exist. So I haven't added any new tests, I'm not super familiar with Go so that's also a reason why I haven't done it.

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

aeneasr · 2021-09-15T09:36:54Z

identity/handler.go

@@ -220,7 +220,8 @@ func (h *Handler) create(w http.ResponseWriter, r *http.Request, _ httprouter.Pa
 		return
 	}

-	i := &Identity{SchemaID: cr.SchemaID, Traits: []byte(cr.Traits), State: StateActive, StateChangedAt: sqlxx.NullTime(time.Now())}
+	var time = sqlxx.NullTime(time.Now())


In go it is considered bad practice to have variable names overshadow package names, so it’s best to rename this:

Suggested change

var time = sqlxx.NullTime(time.Now())

stateChangedAt := sqlxx.NullTime(time.Now())

aeneasr · 2021-09-15T09:37:31Z

identity/handler.go

@@ -318,8 +319,10 @@ func (h *Handler) update(w http.ResponseWriter, r *http.Request, ps httprouter.P
 			h.r.Writer().WriteError(w, r, herodot.ErrBadRequest.WithReasonf("%s", err).WithWrap(err))
 		}

+		var time = sqlxx.NullTime(time.Now())


Suggested change

var time = sqlxx.NullTime(time.Now())

stateChangedAt := sqlxx.NullTime(time.Now())

aeneasr · 2021-09-15T09:37:45Z

identity/handler_test.go

@@ -164,10 +164,11 @@ func TestHandler(t *testing.T) {
 			for name, ts := range map[string]*httptest.Server{"public": publicTS, "admin": adminTS} {
 				t.Run("endpoint="+name, func(t *testing.T) {
 					res := send(t, ts, "POST", "/identities", http.StatusCreated, json.RawMessage(`{"traits": {"bar":"baz"}}`))
+					var time = sqlxx.NullTime(res.Get("state_changed_at").Time())


Same here :)

aeneasr · 2021-09-15T09:37:55Z

identity/identity.go

@@ -208,14 +208,15 @@ func NewIdentity(traitsSchemaID string) *Identity {
 		traitsSchemaID = config.DefaultIdentityTraitsSchemaID
 	}

+	var time = sqlxx.NullTime(time.Now())


And here :)

aeneasr · 2021-09-15T09:38:50Z

selfservice/hook/verification_test.go

@@ -59,9 +59,11 @@ func TestVerifier(t *testing.T) {
 			actual, err = reg.IdentityPool().FindVerifiableAddressByValue(context.Background(), identity.VerifiableAddressTypeEmail, "baz@ory.sh")
 			require.NoError(t, err)
 			assert.EqualValues(t, "baz@ory.sh", actual.Value)
+
+			var time = sqlxx.NullTime(time.Now())


And here :)

aeneasr · 2021-09-15T09:39:06Z

selfservice/strategy/link/strategy_verification.go

@@ -238,7 +238,8 @@ func (s *Strategy) verificationUseToken(w http.ResponseWriter, r *http.Request,

 	address := token.VerifiableAddress
 	address.Verified = true
-	address.VerifiedAt = sqlxx.NullTime(time.Now().UTC())
+	var time = sqlxx.NullTime(time.Now().UTC())
+	address.VerifiedAt = &time


aeneasr · 2021-09-15T09:39:22Z

test/e2e/cypress/helpers/index.js

@@ -13,7 +13,7 @@ const assertVerifiableAddress = ({ isVerified, email }) => ({ identity }) => {
  if (isVerified) {
    expect(address.verified_at).to.not.be.null
  } else {
-    expect(address.verified_at).to.be.null
+    expect(address).to.not.have.property('verified_at')


Signed-off-by: Jakob Sinclair <sinclair.jakob@mailbox.org>

aeneasr

Awesome, thank you! 🎉 Your contribution makes Ory better :)

AntiSC2 requested review from aeneasr and zepatrik as code owners September 7, 2021 21:56

aeneasr requested changes Sep 8, 2021

View reviewed changes

AntiSC2 force-pushed the nulltime-nullable branch 2 times, most recently from 3407d76 to 873af61 Compare September 14, 2021 16:42

AntiSC2 changed the title ~~fix: mark NullTime as nullable in API spec~~ fix: omitempty for VerifiedAt and StateChangedAt Sep 14, 2021

AntiSC2 force-pushed the nulltime-nullable branch from 873af61 to abb2023 Compare September 14, 2021 17:07

AntiSC2 requested a review from aeneasr September 14, 2021 18:30

aeneasr requested changes Sep 15, 2021

View reviewed changes

AntiSC2 force-pushed the nulltime-nullable branch from abb2023 to adcf8ad Compare September 16, 2021 00:54

fix: omitempty for VerifiedAt and StateChangedAt

adcf8ad

Signed-off-by: Jakob Sinclair <sinclair.jakob@mailbox.org>

AntiSC2 requested a review from aeneasr September 16, 2021 01:22

aeneasr approved these changes Sep 16, 2021

View reviewed changes

aeneasr merged commit bf2ec6e into ory:master Sep 16, 2021

afreakk mentioned this pull request Oct 27, 2021

[Kratos dotnet SDK] VerifiableAddress.VerifiedAt is null ory/sdk#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: omitempty for VerifiedAt and StateChangedAt #1736

fix: omitempty for VerifiedAt and StateChangedAt #1736

AntiSC2 commented Sep 7, 2021 •

edited

codecov bot commented Sep 7, 2021 •

edited

aeneasr left a comment

aeneasr Sep 8, 2021

aeneasr Sep 8, 2021

AntiSC2 commented Sep 12, 2021

aeneasr commented Sep 14, 2021

AntiSC2 commented Sep 14, 2021

aeneasr left a comment

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr Sep 15, 2021

aeneasr left a comment

	- op: add
	path: /components/schemas/uiNodeInputAttributes/properties/value/nullable
	value: true

	var time = sqlxx.NullTime(time.Now())
	stateChangedAt := sqlxx.NullTime(time.Now())

fix: omitempty for VerifiedAt and StateChangedAt #1736

fix: omitempty for VerifiedAt and StateChangedAt #1736

Conversation

AntiSC2 commented Sep 7, 2021 • edited

Related issue(s)

Checklist

Further Comments

codecov bot commented Sep 7, 2021 • edited

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntiSC2 commented Sep 12, 2021

aeneasr commented Sep 14, 2021

AntiSC2 commented Sep 14, 2021

aeneasr 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

aeneasr left a comment

Choose a reason for hiding this comment

AntiSC2 commented Sep 7, 2021 •

edited

codecov bot commented Sep 7, 2021 •

edited