Skip to content

feat: Add context-independent validation for id references - #407

Merged
nickevansuk merged 11 commits into
masterfrom
feature/id-reference-validation
Jul 16, 2022
Merged

feat: Add context-independent validation for id references#407
nickevansuk merged 11 commits into
masterfrom
feature/id-reference-validation

Conversation

@civsiv

@civsivcivsiv commented Apr 28, 2022•
edited by nickevansuk
Loading

Copy link
Copy Markdown
Contributor

Fixes: #402

lukehesluke
lukehesluke requested changes May 3, 2022
View reviewed changes
Comment threadsrc/rules/core/no-id-references-for-responses-rule.js Outdated
Comment threadsrc/rules/core/no-id-references-for-responses-rule.js Outdated
Comment threadsrc/rules/core/id-references-for-requests-rule.js Outdated
Comment threadsrc/rules/core/no-id-references-for-responses-rule.js Outdated
@civsivcivsiv mentioned this pull request May 5, 2022
Open
nickevansuk
nickevansuk requested changes May 31, 2022
View reviewed changes

@nickevansuknickevansuk left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look great! Just one design suggestion

Comment threadsrc/rules/core/id-references-for-requests-rule.js Outdated
'BOrderProposalRequest',
'OrderPatch',
];
this.targetFields = { OrderItem: ['acceptedOffer', 'orderedItem'] };

@nickevansuknickevansukMay 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this more generically applicable, it would be good to drive this from the data models.

For example we already have this: https://github.com/openactive/data-models/blob/9bc59064c0f0cb822388212cc79049dbdb5c9893/versions/2.x/models/Order.json#L163-L164

"allowReferencing": true,

So would be good to add referencedFields alongside these:

https://github.com/openactive/data-models/blob/9bc59064c0f0cb822388212cc79049dbdb5c9893/versions/2.x/models/Order.json#L41-L46

"orderProposalRequest": {
"requiredFields": [
"type",
"orderProposalVersion"
],
"recommendedFields": [],

A good hint for how to do this can be found here:

data-model-validator/src/rules/core/required-fields-rule.js

Line 46 in 64227a0

constrequiredFields=node.model.getRequiredFields(node.options.validationMode,node.name);

civsiv
civsiv commented Jun 7, 2022
View reviewed changes
Comment threadpackage.json Outdated
nickevansuk
nickevansuk approved these changes Jul 16, 2022
View reviewed changes

@nickevansuknickevansuk left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have refactored to separate the request/response concerns (which are already covered by the validation mode in the data-models)

@nickevansuknickevansuk changed the title feat: add validation for id references for requests only and objects …feat: Add context-independent validation for id referencesJul 16, 2022
@nickevansuk
nickevansuk merged commit 5363d23 into masterJul 16, 2022
@nickevansuk
nickevansuk deleted the feature/id-reference-validation branch July 16, 2022 00:05
@nickevansuknickevansuk mentioned this pull request Jul 16, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukeheslukelukeheslukelukehesluke approved these changes
@nickevansuknickevansuknickevansuk approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Validate @id references in Booking API context

3 participants

@civsiv@lukehesluke@nickevansuk