-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @Blckbrry-Pi and the rest of your teammates on Graphite |
06cf55a
to
d2012c6
Compare
ae1fc49
to
7d177f8
Compare
d2012c6
to
2832c4c
Compare
7d177f8
to
30af057
Compare
2832c4c
to
bbb15a8
Compare
30af057
to
553a363
Compare
bbb15a8
to
4411d94
Compare
aa4b390
to
c02a755
Compare
auth_provider
moduleidentites
module
c02a755
to
efc852f
Compare
4411d94
to
f3e0b8d
Compare
efc852f
to
bfd5d25
Compare
f3e0b8d
to
ad062ed
Compare
bfd5d25
to
ce34e98
Compare
modules/identities/scripts/get.ts
Outdated
return { data: null }; | ||
} | ||
|
||
const { uniqueData, additionalData } = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this because of our ajv validation issue or something else? ideally we don't have to write ugly code like this.
this might already be fixed on main with the zod migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because uniqueData
and additionalData
are JSON fields in the psql database.
The null reassignment shouldn't be necessary, that was an accident.
However, it doesn't guarantee there's a matching row, and the JSON type doesn't guarantee it's a non-nullable object.
This essentially just says invalid/missing entries have no data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am i missing something: if there's no matching row, identities will be null. it's not going to return an object with undefined properties. our db constraint has not null
on jsonb so all of this code is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sadly is not. The DB constraint says that if there is a row, there is a valid JSON value in that column. null
is a valid JSON value, along with "hello"
, 42
, etc.
Prisma ensures that the value returned will be JSON, but not whether or not it's an object or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify: there is no case where we intentionally set anything that's not an object to the additional data, right? if that's a case, let's write an assertion using the deno assert lib (which throws an internal error, not runtimeerror) & crash instead of failing silentl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise, some downstream script will likely break from unintended state afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Will do!
await ctx.modules.rateLimit.throttlePublic({}); | ||
|
||
// Ensure the user token is valid and get the user ID | ||
const { userId } = await ctx.modules.users.authenticateToken({ userToken: req.userToken } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should let the caller authenticate the token and pass a raw user id. allows for better composability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a public script— are you sure we want that to be public by user ID?
7a38516
to
a119950
Compare
modules/identities/scripts/get.ts
Outdated
return { data: null }; | ||
} | ||
|
||
const { uniqueData, additionalData } = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am i missing something: if there's no matching row, identities will be null. it's not going to return an object with undefined properties. our db constraint has not null
on jsonb so all of this code is redundant.
7a04e0a
to
68f1b5e
Compare
modules/identities/scripts/get.ts
Outdated
return { data: null }; | ||
} | ||
|
||
const { uniqueData, additionalData } = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify: there is no case where we intentionally set anything that's not an object to the additional data, right? if that's a case, let's write an assertion using the deno assert lib (which throws an internal error, not runtimeerror) & crash instead of failing silentl.
modules/identities/scripts/get.ts
Outdated
return { data: null }; | ||
} | ||
|
||
const { uniqueData, additionalData } = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise, some downstream script will likely break from unintended state afaik.
ad062ed
to
1caae6f
Compare
68f1b5e
to
faed173
Compare
Merge activity
|
faed173
to
171fdbe
Compare
No description provided.