-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom schema title for POST requests #3716
Conversation
Discussion point:
This introduces a small inconsistency in schema titles. Do we mind? Please:
/cc @strongloop/loopback-next |
Where is |
Thank you @hacksparrow for the comment.
I see two options now:
|
I don't feel any inconsistency in I understand |
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.
Mostly look good to me, just a few comments.
} | ||
|
||
const schema = getJsonSchema(TestModel, { | ||
title: 'NewTestModel', |
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.
IIUC, the default format for the schema title is New
+ModelName
. And if we don't specified the title, it would be NewTestModel
in this case.
The title of this test is uses custom title when provided by user
. It replaces the title from IgnoredTitle
to 'custom title' NewTestModel
. But IgnoredTitle
looks more like a custom title to me in this case.
Is it possible to change it to make the intention more clear?
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.
@agnes512 Good point!
Let me clarify: The default format for the schema title would be IgnoredTitleExcluding['id']
.
IgnoredTitle
comes from the model-level setting, see https://github.com/strongloop/loopback-next/blob/5bebb5b41e6ce47fcfe7b2b84428b5c7acf3ef64/packages/repository-json-schema/src/build-schema.ts#L289Excluding['id']
is the suffix based onoptions.exclude
, see https://github.com/strongloop/loopback-next/blob/5bebb5b41e6ce47fcfe7b2b84428b5c7acf3ef64/packages/repository-json-schema/src/build-schema.ts#L300-L302
Model-level title
setting IgnoredTitle
applies to all calls of getJsonSchema
where no options.title
is provided. In my use case, I want to provide a different title in each getJsonSchema
call.
Maybe I should rework this test into two? One verifying that model-level title is ignored, another verifying that suffix computed from options is ignored too?
What else can I do to make the intention more clear? If it's enough to improve the test name, could you please suggest a better one?
Thanks! 🙇
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 discussion here 👏
Some personal suggestion: maybe split it into two test cases like:
- title in decorator > title inferred from model class name: verify the
title
in@model()
overrides the title inferred from model class name. no options involved in this test. - title in options > title in decorator: verify the
title
inoptions
overrides thetitle
in@model()
. And obviously option also overrides the one inferred from the model class(proved by the 1st test)
As a summary:
title in options > title in decorator > title inferred from model class name
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.
Janny's suggestion looks good to me.
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.
Done in 3d3e79b.
While improving the tests, I modified my new tests to call modelToJsonSchema
instead of getJsonSchema
, because they are not relying on caching behavior of getJsonSchema
.
@bajtos, for consistency's sake, I think it might be better to have |
@bajtos so just to confirm, if the user chooses to use this And I agree with @hacksparrow's point; it feels as though the beginning part is describing whether it's |
Yes, that's my intention 👍
That's a great way how to explain the schema naming convention 👍 |
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.
The change in PR looks reasonable to me, I left my understanding and suggestion in https://github.com/strongloop/loopback-next/pull/3716/files#r323856015, I feel it would be good to document how title
from different places take precedence over each other :)
I have realized that my original implementation had a flaw in the way how the schema is cached. I added 84db706 to fix it. @jannyHou @agnes512 @nabdelgadir LGTY now? |
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.
👍
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.
LGTM 👍
packages/cli/generators/relation/templates/controller-relation-template-has-many.ts.ejs
Show resolved
Hide resolved
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.
👏 cool
Signed-off-by: Miroslav Bajtoš <[email protected]>
…st bodies Signed-off-by: Miroslav Bajtoš <[email protected]>
84db706
to
31ddb95
Compare
While reviewing #3698, #3504 and #1466, it occurred to me that schema titles like
TodoExcluding[id]
orProductPartialExcluding[id,_rev]
are not very friendly to human consumers.In this pull request, I am introducing a new JsonSchemaOption
title
that allows developer to specify a title that described the intent (data of a new Todo to be created -NewTodo
) instead of the implementation details (Todo data excludingid
property).The second commit updates controller templates and
rest-crud
package to leverage this new schema option for POST requests./cc @ericzon @remkohdev
IMPORTANT: Although this change is very likely to fix the issues described in #3698, #3504 and #1466 for newly created controllers, it's not meant as a replacement for #3504. I'd like #3504 to be landed, because it solves the problems for existing applications and users that decide to not use the
title
option for whatever reasons.Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈