Skip to content
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

node rpc: validateresource #429

Merged
merged 5 commits into from
Apr 19, 2020

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Apr 16, 2020

This PR adds a new Node RPC method called validateresource. It is useful for users who are in the process of creating their resource JSON to use with sendupdate. This PR adds error messages in the fromJSON methods of each of the records found in lib/dns/resource.js.

The RPC accepts a single object with the records property, which must be an array of objects.
Eventually, it could also accept a version property if another Resource version is implemented.

The error messages note the invalid JSON property name and the record type. This is so that good error messages can be returned to users. The error messages could be made generic (not including the record type in the error message) but that would mean that the RPC method handler itself would have to hold logic similar to Resource.fromJSON and keep track of which record failed its fromJSON. I don't like this approach as much since any new resource type will require its fromJSON logic to be added to the RPC method handler but am willing to update this PR if people are strongly opinionated in that way.

Closes #139

lib/node/rpc.js Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member

Looks pretty good, playing with the branch locally and might add a test.

Do you think some of these error message could be even more specific?
e.g.
TXT record "txt" must be an array
Resource "records" must be an array
Resource "records" items must be objects
DS record keyType must be int < 0xffff
DS record "digest" must be string
etc

lib/dns/resource.js Outdated Show resolved Hide resolved
@boymanjor
Copy link
Contributor

Looks pretty good, playing with the branch locally and might add a test.

Do you think some of these error message could be even more specific?
e.g.
TXT record "txt" must be an array
Resource "records" must be an array
Resource "records" items must be objects
DS record keyType must be int < 0xffff
DS record "digest" must be string
etc

How about Invalid DS record. Property "json" must be an Array.

@tynes tynes force-pushed the rpc-validate-resource branch from 06a704f to 7bbfaa4 Compare April 16, 2020 22:01
@tynes
Copy link
Contributor Author

tynes commented Apr 16, 2020

New commits pushed up, I followed similar error messaging to @boymanjor's suggestion here:
DS record "digest" must be string

@tynes tynes changed the title Node RPC verifyresource Node RPC validateresource Apr 16, 2020
@tynes tynes force-pushed the rpc-validate-resource branch 2 times, most recently from 9d4d64e to 39df65f Compare April 16, 2020 23:01
@tynes
Copy link
Contributor Author

tynes commented Apr 16, 2020

This is ready for another round of review. I wrote tests for both success and failure cases.

lib/dns/resource.js Outdated Show resolved Hide resolved
@tynes tynes force-pushed the rpc-validate-resource branch from 39df65f to 0b147e1 Compare April 16, 2020 23:35
@codecov-io
Copy link

Codecov Report

Merging #429 into master will increase coverage by 0.01%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   61.97%   61.98%   +0.01%     
==========================================
  Files         129      129              
  Lines       34831    34843      +12     
  Branches     5918     5920       +2     
==========================================
+ Hits        21587    21598      +11     
- Misses      13244    13245       +1     
Impacted Files Coverage Δ
lib/node/rpc.js 28.30% <83.33%> (+0.66%) ⬆️
lib/dns/resource.js 84.77% <100.00%> (ø)
lib/utils/binary.js 56.41% <0.00%> (-2.57%) ⬇️
lib/protocol/consensus.js 87.75% <0.00%> (-1.03%) ⬇️
lib/covenants/rules.js 73.04% <0.00%> (-0.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d5ab27...0b147e1. Read the comment docs.

lib/node/rpc.js Outdated Show resolved Hide resolved
@pinheadmz
Copy link
Member

Great work on this dude, thanks for the thorough test. Only thing left for me is the help message.

tynes added 5 commits April 17, 2020 18:15
This new node RPC will verify Resource json.
It will be useful for users who are having trouble
building the correct Resource.
These new error messages will help users
more easily craft Resources. The error messages
will be returned via the `validateresource` RPC.
Test the success cases and the failure cases.
@tynes tynes force-pushed the rpc-validate-resource branch from c20ac1a to d5d9821 Compare April 18, 2020 01:15
@tynes tynes changed the title Node RPC validateresource node rpc: validateresource Apr 19, 2020
@pinheadmz
Copy link
Member

ACK @ d5d9821

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validateresource Node RPC
4 participants