fix: add missing extends field to Role types#121
Conversation
zeevmoney
left a comment
There was a problem hiding this comment.
The extends addition itself is correct — it matches the backend exactly (extends: Optional[list[str]] on the shared _Editable base, inherited by RoleCreate/Read/Update; permit-backend .../schemas/schema_role.py:56-60). The blockers are scope and method, not the field.
| * @type {Array<string>} | ||
| * @memberof RoleRead | ||
| */ | ||
| extends?: Array<string>; |
There was a problem hiding this comment.
[MEDIUM] Hand-editing generated files will be clobbered on regen (applies to role-create/role-update too)
These files carry the 'auto generated by OpenAPI Generator — Do not edit the class manually' header and are produced by yarn generate-openapi-client from the live spec (package.json:54). Since extends is already in the backend/spec, a manual edit diverges from the generator and is overwritten on the next regeneration.
Suggestion: Run yarn generate-openapi-client and commit the regenerated output — it picks up extends (and any other drift) faithfully and survives regen.
| if (!config.log.json) { | ||
| return pino( | ||
| options, | ||
| pino.transport({ target: 'pino-pretty', options: { levelFirst: true } }), |
There was a problem hiding this comment.
[HIGH] Out-of-scope logger rewrite changes default behavior and adds a fragile transport
This is unrelated to the extends fix. The default config is json: false (config.ts:133), and this routes that default path through pino.transport({ target: 'pino-pretty' }). Two problems: (1) it changes the default log format for every consumer (today prettyPrint is a no-op on pino 8, so output is currently JSON); (2) pino.transport spawns a worker thread that resolves pino-pretty at runtime — in bundled (tsup), ESM, or serverless environments this commonly throws at logger creation, which runs during client init (index.ts), so it can break SDK startup, not just logging. No tests.
Suggestion: Drop the logger.ts change from this PR. If the pino-8 prettyPrint deprecation needs fixing, do it in a dedicated, tested PR and verify the transport resolves under the shipped tsup bundle.
| "path-to-regexp": "^6.2.1", | ||
| "pino": "8.11.0", | ||
| "pino-pretty": "10.2.0", | ||
| "axios": "^1.13.5", |
There was a problem hiding this comment.
[MEDIUM] Unrelated dependency bumps + large lockfile churn in a type-only PR
This bumps axios ^1.7.4→^1.13.5, lodash, path-to-regexp, and moves pino/pino-pretty from pinned exact versions to caret ranges, plus 576/363 lines of yarn.lock churn — none related to adding extends, and likely to conflict with the approved #127 (which also touches packaging).
Suggestion: Drop the dep/lockfile changes here; if a security bump (axios) is wanted, do it in its own PR. Keep pino/pino-pretty pinned unless something actually requires a newer version.
There was a problem hiding this comment.
Pull request overview
This PR aims to align the SDK’s generated Role TypeScript interfaces with the Permit.io OpenAPI spec by adding the missing extends?: Array<string> field, enabling role inheritance in create/update flows and typed access when reading roles. It also includes runtime dependency upgrades and a logging implementation change to support newer pino/pino-pretty behavior.
Changes:
- Add
extends?: Array<string>toRoleRead,RoleCreate, andRoleUpdateinterfaces. - Update logger initialization to use
pino.transportwithpino-prettywhenconfig.log.jsonis false. - Bump multiple runtime dependencies (axios/lodash/path-to-regexp/pino/pino-pretty) and regenerate
yarn.lockaccordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Lockfile updates reflecting dependency upgrades and transitive changes. |
| package.json | Updates runtime dependency versions (axios/lodash/path-to-regexp/pino/pino-pretty). |
| src/logger.ts | Reworks logger creation to use pino-pretty transport instead of deprecated prettyPrint. |
| src/openapi/types/role-create.ts | Adds extends?: Array<string> to role creation type. |
| src/openapi/types/role-read.ts | Adds extends?: Array<string> to role read type. |
| src/openapi/types/role-update.ts | Adds extends?: Array<string> to role update type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const options: pino.LoggerOptions = { | ||
| level: config.log.level, | ||
| prettyPrint: config.log.json ? { levelFirst: true } : false, | ||
| base: { label: config.log.label }, | ||
| timestamp: pino.stdTimeFunctions.isoTime, | ||
| }); | ||
| }; | ||
|
|
||
| if (!config.log.json) { | ||
| return pino( | ||
| options, | ||
| pino.transport({ target: 'pino-pretty', options: { levelFirst: true } }), | ||
| ); | ||
| } | ||
|
|
||
| return pino(options); |
| "@bitauth/libauth": "^1.17.1", | ||
| "axios": "^1.7.4", | ||
| "lodash": "^4.17.21", | ||
| "path-to-regexp": "^6.2.1", | ||
| "pino": "8.11.0", | ||
| "pino-pretty": "10.2.0", | ||
| "axios": "^1.13.5", | ||
| "lodash": "^4.17.23", | ||
| "path-to-regexp": "^6.3.0", | ||
| "pino": "^8.21.0", | ||
| "pino-pretty": "^10.3.1", | ||
| "require-in-the-middle": "^5.1.0", |
| /** | ||
| * list of role keys that define what roles this role extends. In other words: this role will automatically inherit all the permissions of the given roles in this list. | ||
| * @type {Array<string>} | ||
| * @memberof RoleCreate | ||
| */ | ||
| extends?: Array<string>; |
| /** | ||
| * list of role keys that define what roles this role extends. In other words: this role will automatically inherit all the permissions of the given roles in this list. | ||
| * @type {Array<string>} | ||
| * @memberof RoleRead | ||
| */ | ||
| extends?: Array<string>; |
| /** | ||
| * list of role keys that define what roles this role extends. In other words: this role will automatically inherit all the permissions of the given roles in this list. | ||
| * @type {Array<string>} | ||
| * @memberof RoleUpdate | ||
| */ | ||
| extends?: Array<string>; |
| if (!config.log.json) { | ||
| return pino( | ||
| options, | ||
| pino.transport({ target: 'pino-pretty', options: { levelFirst: true } }), | ||
| ); | ||
| } |
The Permit API and OpenAPI spec define extends (Array<string>) on the role schemas, but it is missing from the generated TypeScript types, so consumers cannot type-safely set or read role inheritance. Add extends?: Array<string> to RoleCreate/Read/Update and ResourceRoleCreate/Read/Update, matching the generator's emitted style and field position so a future regeneration is a no-op. Add a unit test pinning the field on all six types; it fails to compile if any addition is reverted. The canonical fix (regenerate the client) is currently blocked: the pinned generator 6.2.1 cannot read the now-OpenAPI-3.1 spec and regenerates an all-any client (see permitio#130).
c57d16c to
023f1ce
Compare
|
Thanks for the review. Agreed on scope and the missing test, both fixed below. One note on the regenerate suggestion. Scope: dropped the Test: added On regenerating instead of hand-editing: I tried that first, and I also extended it to the |
Bug fix. Adds the missing
extendsfield to the generated Role and ResourceRole types so SDK consumers can use role inheritance with type safety.The Permit API and the OpenAPI spec define
extends(Array<string>) on the role schemas, and the API returns it, but it is missing from the generated TypeScript types. Consumers cannot set or read role inheritance without a type error:The same gap affects the resource-scoped roles (
permit.api.resourceRoles.*).This replaces the earlier version of the PR, which bundled unrelated changes. See "Other information".
extends?: Array<string>is added to the six generated role interfaces:RoleCreate,RoleRead,RoleUpdateResourceRoleCreate,ResourceRoleRead,ResourceRoleUpdateThe member matches the generator output after the repo's prettier post-processing: the live-spec doc comment, positioned between
attributesandgranted_to, so a future regeneration reproduces this exactextendsmember as a no-op. A full regeneration would also pick up unrelated spec drift on these schemas, such as updatedgranted_totyping and thev1compat_*fields; that re-baseline is tracked separately in #130. A unit test (src/tests/unit/role-extends.spec.ts) pins the field on all six types for create, update, and read; it fails to compile if any of the additions is reverted.RoleCreate/ResourceRoleCreateacceptextendsRoleRead.extends/ResourceRoleRead.extendstypedArray<string>RoleUpdate/ResourceRoleUpdateacceptextendsyarn buildyarn lintyarn test:unitrole-extendsrole-extends.spec.tsfails to compile (4 assignability + 6 member-access errors)Scope. This PR is now only the
extendstype addition and its test. The earlier version also rewrotesrc/logger.ts(pino transport) and bumped several runtime dependencies with a large lockfile change. Both are unrelated toextends, were flagged in review, and are dropped here.On hand-editing a generated file. The review suggested running
yarn generate-openapi-clientand committing the regenerated output instead of a hand-edit. That path is currently broken. The pinned generator (6.2.1) cannot read the live spec, which is now OpenAPI 3.1.0, so it regenerates an all-any, type-erased client (247 of 319 type files) and still exits 0. Regenerating today would replace the whole typed client withany, which is worse than a missing field. I reported that separately in #130 with a reproduction and options. Targeted hand-edits tosrc/openapi/types/*.tsare accepted practice in this repo: be9b59a (Add created_at and updated_at to userRead schema) added two fields touser-read.tsby hand, and c502f4e (add 2 more checks for derived roles settings) added an import and a field toderived-role-rule-create.ts, both single-file edits to files carrying the same "Do not edit the class manually" header. Until #130 is fixed, hand-adding the field is the lowest-risk option, and because it matches generator output it folds into the next clean regeneration as a no-op. The new test guards against silent loss in the meantime.Not included here, on purpose: a full client regeneration (blocked by #130), the
v1compat_*fields, which the spec also places on most of these schemas but which the committed client predates (a separate missing-field gap, intentionally out of scope here), and any change to non-role types.