fix(document): Enforce schema when loading genesis record #472
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,19 +33,27 @@ const createIPFS =(overrideConfig: Record<string, unknown> = {}): Promise<IPFSAp | |
| return IPFS.create(config) | ||
| } | ||
|
|
||
| const registerChangeListener = async (doc: any): Promise<void> => { | ||
| return new Promise(resolve => { | ||
| doc.on('change', () => { | ||
| resolve() | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Registers a listener for change notifications on a document, instructs the anchor service to | ||
| * perform an anchor, then waits for the change listener to resolve, indicating that the document | ||
| * got anchored. | ||
| * @param ceramic | ||
| * @param doc | ||
| */ | ||
| const anchorDoc = async (ceramic: Ceramic, doc: any): Promise<void> => { | ||
| const changeHandle = await registerChangeListener(doc) | ||
| await ceramic.context.anchorService.anchor() | ||
| await changeHandle | ||
| } | ||
|
|
||
| describe('Ceramic API', () => { | ||
| jest.setTimeout(15000) | ||
| let ipfs: IPFSApi; | ||
|
|
@@ -67,6 +75,7 @@ describe('Ceramic API', () => { | |
|
|
||
| const createCeramic = async (c: CeramicConfig = {}): Promise<Ceramic> => { | ||
| c.topic = topic | ||
| c.anchorOnRequest = false | ||
| const ceramic = await Ceramic.create(ipfs, c) | ||
|
|
||
| const config = { | ||
|
|
@@ -108,7 +117,7 @@ describe('Ceramic API', () => { | |
| }) | ||
|
|
||
| // wait for anchor (new version) | ||
| await anchorDoc(ceramic, docOg) | ||
|
|
||
| expect(docOg.state.log.length).toEqual(2) | ||
| expect(docOg.content).toEqual({ test: 321 }) | ||
|
|
@@ -119,7 +128,7 @@ describe('Ceramic API', () => { | |
| await docOg.change({ content: { test: 'abcde' } }) | ||
|
|
||
| // wait for anchor (new version) | ||
| await anchorDoc(ceramic, docOg) | ||
|
|
||
| expect(docOg.state.log.length).toEqual(4) | ||
| expect(docOg.content).toEqual({ test: 'abcde' }) | ||
|
|
@@ -339,7 +348,7 @@ describe('Ceramic API', () => { | |
| content: { a: 1 }, | ||
| } | ||
| const doc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, tileDocParams) | ||
| await anchorDoc(ceramic, doc) | ||
|
|
||
| // Create schema that enforces that the content value is a string, which would reject | ||
| // the document created above. | ||
|
|
@@ -348,7 +357,7 @@ describe('Ceramic API', () => { | |
| metadata: { controllers: [controller] } | ||
| }) | ||
| // wait for anchor | ||
| await anchorDoc(ceramic, schemaDoc) | ||
| expect(schemaDoc.state.anchorStatus).toEqual(AnchorStatus.ANCHORED) | ||
|
|
||
| // Update the schema to expect a number, so now the original doc should conform to the new | ||
|
|
@@ -357,7 +366,7 @@ describe('Ceramic API', () => { | |
| updatedSchema.additionalProperties.type = "number" | ||
| await schemaDoc.change({content: updatedSchema}) | ||
| // wait for anchor | ||
| await anchorDoc(ceramic, schemaDoc) | ||
| expect(schemaDoc.state.anchorStatus).toEqual(AnchorStatus.ANCHORED) | ||
|
|
||
| // Test that we can assign the updated schema to the document without error. | ||
|
|
@@ -366,7 +375,7 @@ describe('Ceramic API', () => { | |
| controllers: [controller], schema: schemaDoc.id.toString() | ||
| } | ||
| }) | ||
| await anchorDoc(ceramic, doc) | ||
| expect(doc.content).toEqual({ a: 1 }) | ||
|
|
||
| // Test that we can reload the document without issue | ||
|
|
@@ -377,74 +386,6 @@ describe('Ceramic API', () => { | |
| await ceramic.close() | ||
| }) | ||
|
|
||
|
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 |
||
| it('Pin schema to a specific version', async () => { | ||
| ceramic = await createCeramic() | ||
|
|
||
|
|
@@ -455,10 +396,11 @@ describe('Ceramic API', () => { | |
| metadata: { | ||
| controllers: [controller] | ||
| }, | ||
| content: { stuff: 'a' }, | ||
| } | ||
| const doc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, tileDocParams) | ||
| await anchorDoc(ceramic, doc) | ||
| expect(doc.content).toEqual({ stuff: 'a' }) | ||
|
|
||
| // Create schema that enforces that the content value is a string | ||
| const schemaDoc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, { | ||
|
|
@@ -467,52 +409,48 @@ describe('Ceramic API', () => { | |
| }) | ||
|
|
||
| // wait for anchor | ||
| await anchorDoc(ceramic, schemaDoc) | ||
| expect(schemaDoc.state.anchorStatus).toEqual(AnchorStatus.ANCHORED) | ||
| const schemaV0Id = DocID.fromBytes(schemaDoc.id.bytes, schemaDoc.tip.toString()) | ||
|
|
||
| // Assign the schema to the conforming document, specifying current version of the schema explicitly | ||
| await doc.change({ | ||
| metadata: { controllers: [controller], schema: schemaV0Id.toString() }, | ||
| content: {stuff: 'b'} | ||
| }) | ||
| await anchorDoc(ceramic, doc) | ||
| expect(doc.content).toEqual({ stuff: 'b' }) | ||
| expect(doc.metadata.schema).toEqual(schemaV0Id.toString()) | ||
|
|
||
| // Update schema so that existing doc no longer conforms | ||
| const updatedSchema = cloneDeep(stringMapSchema) | ||
| updatedSchema.additionalProperties.type = "number" | ||
| await schemaDoc.change({content: updatedSchema}) | ||
| await anchorDoc(ceramic, schemaDoc) | ||
|
|
||
| expect(doc.metadata.schema.toString()).toEqual(schemaV0Id.toString()) | ||
|
|
||
| // Test that we can update the existing document according to the original schema when taking | ||
| // the schema docID from the existing document. | ||
| await doc.change({ | ||
| content: { stuff: 'c' }, | ||
| metadata: { controllers: [controller], schema: doc.metadata.schema.toString() } | ||
| }) | ||
| await anchorDoc(ceramic, doc) | ||
| expect(doc.content).toEqual({ stuff: 'c' }) | ||
|
|
||
| // Test that updating the existing document fails if it doesn't conform to the most recent | ||
| // version of the schema, when specifying just the schema document ID without a version | ||
| try { | ||
| await doc.change({ | ||
| content: {stuff: 'd'}, | ||
| metadata: {controllers: [controller], schema: schemaDoc.id.toString() } | ||
| }) | ||
| throw new Error('Should not be able to update the document with invalid content') | ||
| } catch (e) { | ||
| expect(e.message).toEqual('Validation Error: data[\'stuff\'] should be number') | ||
| } | ||
| expect(doc.content).toEqual({ stuff: 'c' }) | ||
|
|
||
| await ceramic.close() | ||
| }) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay nevermind, I more or less see the problem. Making registerChangeListener async makes the compiler think that saying
await registerChangeListener(doc)returnsvoid, rather thanPromise<void>like it should be. I tried to change the return type toPromise<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 @simonovic86There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, this is OK 👍