-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Format schema with oneOf doesn't work correctly #7055
Comments
Hello, I would like to contribute to resolve this issue. Thanks |
@Franklin-Osede Hi, are you working on this issue? I want to take over it if you're not yet. 😄 |
Hey @mmyyrroonn, it doesn't look like @Franklin-Osede is working on this. Please go ahead and work on this one. We appreciate you wanting to help out. |
@avkos @mconnelly8 Hi, after debugging the code, I found the root cause. The web3.js/packages/web3-utils/src/formatter.ts Line 325 in 89711ab
Here are three ways to fix this bug in my mind.
I would like to know more about this bug and its background before I start my PR. Is there any guideline we should follow for the |
Hey there @mmyyrroonn thanks for taking a look into this :) |
Hi, I'm glad to hear your response. I'm not familiar with the schema. what is the definition of the keyword |
You bring up a really good point, docs would definitely be helpful in this and its something we probably want in the future :)
oneOf allows the formatter match multiple types. In this example,
oneOf allows a schema to be matched by either an array with objects of Honestly you are being a really big help because this has been an issue thats persisted for quite a while 😅 and being able to identify that this is occuring because I think targetting |
Thank you for the examples you provided. After reading them, I would like to confirm a few points:
|
@mmyyrroonn Yes it should only match only one, and I believe that part is written already so you wouldn't need to worry. I think in this case
if i understand the error, |
@luu-alex Let me share more details here for our discussion. web3.js/packages/web3-utils/src/formatter.ts Lines 290 to 293 in 5ad7e5b
The output of findSchemaByDataPath is {"oneOf":[{"format":"string"},{"type":"null"}]} instead of {"format":"string"} , which, in my view, is incorrect. I am unsure if this result aligns with the original intent of the function or if oneOf should be handled correctly here. I will explain the specific logic of this function below.
web3.js/packages/web3-utils/src/formatter.ts Lines 295 to 311 in 5ad7e5b
Since schemaProp is not null and the value is 123, which is not object or array , the above logic would be skipped and go into the convertScalarValue function.web3.js/packages/web3-utils/src/formatter.ts Line 325 in 5ad7e5b
The schemaProp.format is undefined and the convertScalarValue function would go into the error catch path.web3.js/packages/web3-utils/src/formatter.ts Line 134 in 5ad7e5b
Returning to this function, dataPath consists of only one item, so the loop will iterate only once and not twice. In this iteration, previousDataPath is empty, so the logic related to oneOf at line 59 is skipped, and lines 71 and 83 are executed directly. After the loop ends, the function returns. At this point, the web3.js/packages/web3-utils/src/formatter.ts Lines 50 to 86 in 5ad7e5b
I'm unsure if the findSchemaByDataPath function should handle this scenario because in this case, the oneOfPath is also empty.In my view, the handling logic for this example has been overlooked. Perhaps we should modify this logic to include additional handling. web3.js/packages/web3-utils/src/formatter.ts Lines 303 to 306 in 5ad7e5b
let me know what you think 😄 |
Ah I understand it a bit more, sure It makes sense to include this additional handling. Feel free to start on the PR |
working on it |
@luu-alex Hi, I created a PR to demonstrate the fix. Actually, there are two issues: one is the problem I described above. The other is that |
@mmyyrroonn Thanks for the PR! Ill take a look |
Actual Behaviour:
to
is not formattedExpected Behaviour:
to
should be formatted to a stringThe text was updated successfully, but these errors were encountered: