fix: Fixed deployContract to throw error if get additional property #3885
Conversation
Harshdev098
requested review from
petermetz,
takeutak,
izuru0,
TheJagpreet,
VRamakrishna,
sandeepnRES and
outSH
as code owners
There was a problem hiding this comment.
@Harshdev098 Thank you for your contribution! Overall, looking pretty good, but I did have a few minor change requests that you can see above. On top of those, I'll also ask that you amend your commit message to match the text that you've put in the PR description.
If you need help with any of these tasks/updates just let me know I'm happy to help out. If you want to take advantage of that help you can also join the daily pair programming sessions on Discord at 10 AM Pacific Time (runs most weekdays).
...in-ledger-connector-ethereum/src/main/typescript/web-services/deploy-contract-v1-endpoint.ts
Outdated
Show resolved
Hide resolved
...in-ledger-connector-ethereum/src/main/typescript/web-services/deploy-contract-v1-endpoint.ts
Outdated
Show resolved
Hide resolved
...src/test/typescript/integration/geth-contract-deploy-and-invoke-using-json-object-v1.test.ts
Outdated
Show resolved
Hide resolved
|
@petermetz Thanks for the detailed feedback! I have updated the PR with required changes, can you please review it |
|
|
||
| async function preprocessSchema() { | ||
| const resolvedSchema = await RefParser.dereference(OAS); | ||
| function adjustNullable(obj: any) { |
There was a problem hiding this comment.
Instead of any you can accept Record<string, unknown>
| let validateDeployContract: any; | ||
| (async () => { | ||
| const ajv = new Ajv({ | ||
| allErrors: true, | ||
| strict: false, | ||
| allowMatchingProperties: true, | ||
| }); | ||
| const processedSchema = await preprocessSchema(); | ||
| ajv.addSchema(processedSchema, "openapi.json"); | ||
|
|
||
| validateDeployContract = ajv.compile({ | ||
| $ref: "openapi.json#/components/schemas/DeployContractV1Request", | ||
| }); | ||
| })(); |
There was a problem hiding this comment.
I don't think IIFE is safe here, it may not finish until handleRequest is first called. Please use memoization instead (e.g. add function getValidateDeployContract that will compile the schema or return already compiled schema if available.
There was a problem hiding this comment.
@outSH, Done with the changes!
|
@Harshdev098 Sorry for the slow response, LGMT! |
Fix: hyperledger-cacti#3507 - Fixed the deployContract to throw error if get any additional property - Used ajv and to validate the requests - Used json-schema-ref-parser to handle the nullable property in the openAPI as ajv was throwing errors becuase it require a type keyword when using nullable, while openapi.json uses nullable in contexts. So preferred to go with ref-parser instead of updating the openAPI Signed-off-by: Harshdev098 <[email protected]>
|
Thank you for your contribution. @Harshdev098 please update the MR, rebasing it as per your availability. Please note that we will close stale MRs to cleanup in a close future. |
|
@RafaelAPB If there are no conflicts, you can select and click: 
or rebase it manually first (e.g. to see if some optional tests are passing - recommended) 
That way we can save some time and not require more actions from the contributors :) I will not run them now, so you can experiment and see if that's acceptable for you |
|
@outSH not sure that would work. because DCO is failing, and we are trying to priortize rebases over merge commits. |
Fix: #3507
ajvand to validate the requestsjson-schema-ref-parserto handle the nullable property in the openAPI asajvwas throwing errors becuase it require a type keyword when using nullable, while openapi.json uses nullable in contexts. So preferred to go with ref-parser instead of updating the openAPI