zenstackhq / zenstack

Fullstack TypeScript toolkit that enhances Prisma ORM with flexible Authorization layer for RBAC/ABAC/PBAC/ReBAC, offering auto-generated type-safe APIs and frontend hooks.
https://zenstack.dev
MIT License
2.07k stars 89 forks source link

[Feature Request] Mark nonnullable attributes as required in RESTApiHandler get requests #1744

Open thomassnielsen opened 2 weeks ago

thomassnielsen commented 2 weeks ago

When using the RESTApiHandler and OpenAPI plugin, the return types for attributes in a get request are currently all optional (<type> | undefined). Since JSON:API responses always include all the attributes, they should not be marked as optional. This can be achieved with listing the attributes in a required array in the openapi schema.

Basically what we want is for the attributes to go from e.g.

Before

{
  myString: string | undefined
  myNullableString: string | null | undefined
}

After

{
  myString: string
  myNullableString: string | null
}

See Discord discussion for more details.

This change does not apply to RPCApiHandler since it allows selecting which attributes to return, so they're all optional in practice.

ymc9 commented 1 day ago

Hi @thomassnielsen , JSON:API actually has a feature called sparse fieldset that allows selecting fields per type:

https://jsonapi.org/format/#fetching-sparse-fieldsets

It's not implemented by ZenStack yet, but we may add it in the future. Does the current optional typing affect you adversely in some way?

thomassnielsen commented 1 day ago

It's not implemented by ZenStack yet, but we may add it in the future. Does the current optional typing affect you adversely in some way?

We're using Zenstack to remove direct db access for a project and instead going through an API. If Zenstack had non-optional typing we could replace the Prisma calls with a generated API client as a drop in replacement. As it stands, we either need to override the types or rewrite the code to handle optionals.

I think it makes sense for Zenstack to require these fields as long as sparse fieldset is not implemented, and probably also only use optional types when sparse fieldset is in use.

I may have some time to look at implementing this (required fields, not sparse fieldsets yet) tomorrow or next week.

ymc9 commented 15 hours ago

It's not implemented by ZenStack yet, but we may add it in the future. Does the current optional typing affect you adversely in some way?

We're using Zenstack to remove direct db access for a project and instead going through an API. If Zenstack had non-optional typing we could replace the Prisma calls with a generated API client as a drop in replacement. As it stands, we either need to override the types or rewrite the code to handle optionals.

I think it makes sense for Zenstack to require these fields as long as sparse fieldset is not implemented, and probably also only use optional types when sparse fieldset is in use.

I may have some time to look at implementing this (required fields, not sparse fieldsets yet) tomorrow or next week.

I see. Yes, I agree with making the fields non-optional. We can revisit this when implementing the feature. A PR is greatly appreciated as always!