fix(document): Enforce schema when loading genesis record #472
Conversation
| await syncDoc(schemaDoc) | ||
|
|
||
| // Test that we can load the existing document without issue | ||
| const doc2 = await ceramic.loadDocument(doc.id) |
There was a problem hiding this comment.
The whole point of this test was to test the behavior when loading a document that doesn't conform to its schema, but this check here isn't actually telling us anything useful since it's not going through the "real" document loading process, but instead just fetching it out of the local cache here.
To do this test properly it would need to be re-written as an integration test with two ceramic instances, one that creates the non-conforming document and a second that loads it. I'm happy to do that if you think it's worthwhile to add, but in the interest of time I decided to just focus on unit testing the core Document/applyRecord behavior and trust that it continues to work when being driven by the pubsub network
There was a problem hiding this comment.
feel free to add a test into ceramic.test.ts test suite which is already spinning a couple of instances (ipfs+ceramic). It currently serves as a basic integration test for testing propagation between multiple instances. On the other side, it's not crucial to add it as part of this PR, we can open another story for that 👍
| expect(doc.metadata.schema).toEqual(schemaDoc.id.toString()) | ||
|
|
||
| try { | ||
| await Document.load(doc.id, findHandler, dispatcher, pinStore, context, {skipWait:true}) |
There was a problem hiding this comment.
This is the actual test for the core fix in this PR. Without the change to Document.load, this test fails.
|
I feel like I have a much better grasp of how this all works now. My previous statement "we aren't enforcing schema validation when loading documents" was only partially correct. We did indeed have a bug of not enforcing the schema when loading the genesis record, which is what this ticket fixes. But for non-genesis update records, we do actually enforce the schema. The reason I wasn't seeing that in my testing was due to this document cache which causes my test code to not actually truly reload the document. The actual behavior when loading a document where one of the intermediary update records doesn't conform to the schema as specified at the time of that update is that that signedRecord just gets thrown out silently. This means that |
|
Hmm... I'm not sure why circleci failed to run the tests, they pass for me locally. Any ideas how to go about looking into that? @oed @simonovic86? It really would be nice if our test system logged what test was being run before running it, instead of only after... |
|
Yeah, idk why circleci is not showing which test result in an error 🤔 |
It seems that some of the tests are failing in a random manner. I guess that's related with how we're using the |
| @@ -145,6 +145,13 @@ class Document extends EventEmitter { | |||
| doc._doctype.state = await doc._doctypeHandler.applyRecord(record, doc._genesisCid, context) | |||
| } | |||
|
|
|||
| if (validate) { | |||
| const syncDoc = async (doctype: Doctype): Promise<void> => { | ||
| await new Promise(resolve => { | ||
| doctype.on('change', () => { | ||
| const registerChangeListener = function (doc: Doctype): Promise<void> { |
There was a problem hiding this comment.
I hit some weirdness trying to get this to work. I originally had this as an async function, but then waiting on the returned promise in anchorDoc would hang. Removing the 'return' keyword after making it async would get it to stop hanging, but then it would fail on the next line in every test because the document log would be one entry smaller than expected. I definitely think I'm hitting up against some rough edges of the concurrency/execution model of Node.js. Would you be open to taking a look at this together tomorrow morning @simonovic86?
There was a problem hiding this comment.
Okay nevermind, I more or less see the problem. Making registerChangeListener async makes the compiler think that saying await registerChangeListener(doc) returns void, rather than Promise<void> like it should be. I tried to change the return type to Promise<Promise<void>> but that doesn't seem to help, the compiler seems to flatten the stack of Promises. I even found a bug report about it. Anyway I think the right move is just to leave the function synchronous, as I've done here. So I think this should be good as-is, pending your review @simonovic86
No description provided.