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.

Sign up for GitHub

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

Jump to bottom

fix(document): Enforce schema when loading genesis record #472

Merged
merged 4 commits into from
Nov 10, 2020
Merged

fix(document): Enforce schema when loading genesis record #472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa40030
fix(document): Enforce schema when loading genesis record
Nov 5, 2020
879367c
Improve test robustness by manually anchoring
Nov 6, 2020
8431377
Increase robustness of ceramic-anchor-test
Nov 9, 2020
719f127
Change registerChangeListener function in ceramic-api-test to be sync…
Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 10 additions & 81 deletions packages/core/src/__tests__/ceramic-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,74 +377,6 @@ describe('Ceramic API', () => {
await ceramic.close()
})

it('update schema so existing doc no longer conforms', async () => {
ceramic = await createCeramic()

const controller = ceramic.context.did.id

// Create doc with content that has type 'string'.
const tileDocParams: TileParams = {
metadata: {
controllers: [controller]
},
content: { a: 'x' },
}
const doc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, tileDocParams)
await syncDoc(doc)

// Create schema that enforces that the content value is a string
const schemaDoc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, {
content: stringMapSchema,
metadata: { controllers: [controller] }
})
// wait for anchor
await syncDoc(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.
await doc.change({
metadata: {
controllers: [controller], schema: schemaDoc.id.toString()
}
})
await syncDoc(doc)
expect(doc.content).toEqual({ a: 'x' })

// Update schema so that existing doc no longer conforms
const updatedSchema = cloneDeep(stringMapSchema)
updatedSchema.additionalProperties.type = "number"
await schemaDoc.change({content: updatedSchema})
await syncDoc(schemaDoc)

// Test that we can load the existing document without issue
const doc2 = await ceramic.loadDocument(doc.id)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(doc2.content).toEqual(doc.content)

// 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: {a: 'y'},
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[\'a\'] should be number')
}

// Test that we can update the existing document according to the original schema by manually
// specifying the old version of the schema
await doc.change({
content: { a: 'z' },
metadata: { controllers: [controller], schema: schemaV0Id.toString() }
})
await syncDoc(doc)
expect(doc.content).toEqual({ a: 'z' })

await ceramic.close()
})

it('Pin schema to a specific version', async () => {
ceramic = await createCeramic()

Expand All @@ -455,10 +387,11 @@ describe('Ceramic API', () => {
metadata: {
controllers: [controller]
},
content: { a: 'x' },
content: { stuff: 'a' },
}
const doc = await ceramic.createDocument<TileDoctype>(DOCTYPE_TILE, tileDocParams)
await syncDoc(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, {
Expand All @@ -473,12 +406,11 @@ describe('Ceramic API', () => {

// Assign the schema to the conforming document, specifying current version of the schema explicitly
await doc.change({
metadata: {
controllers: [controller], schema: schemaV0Id.toString(),
}
metadata: { controllers: [controller], schema: schemaV0Id.toString() },
content: {stuff: 'b'}
})
await syncDoc(doc)
expect(doc.content).toEqual({ a: 'x' })
expect(doc.content).toEqual({ stuff: 'b' })
expect(doc.metadata.schema).toEqual(schemaV0Id.toString())

// Update schema so that existing doc no longer conforms
Expand All @@ -489,30 +421,27 @@ describe('Ceramic API', () => {

expect(doc.metadata.schema.toString()).toEqual(schemaV0Id.toString())

// Test that we can load the existing document without issue
const doc2 = await ceramic.loadDocument(doc.id)
expect(doc2.content).toEqual(doc.content)
expect(doc2.metadata).toEqual(doc.metadata)

// 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: { a: 'y' },
content: { stuff: 'c' },
metadata: { controllers: [controller], schema: doc.metadata.schema.toString() }
})
await syncDoc(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: {a: 'z'},
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[\'a\'] should be number')
expect(e.message).toEqual('Validation Error: data[\'stuff\'] should be number')
}
expect(doc.content).toEqual({ stuff: 'c' })

await ceramic.close()
})
Expand Down
73 changes: 73 additions & 0 deletions packages/core/src/__tests__/document.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,15 @@ const create = async (params: TileParams, ceramic: Ceramic, context: Context, op
return await ceramic._createDocFromGenesis(record, opts)
}

const stringMapSchema = {
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "StringMap",
"type": "object",
"additionalProperties": {
"type": "string"
}
}

let stateStore: LevelStateStore
let pinStore: PinStore
let pinning: PinningBackend
Expand Down Expand Up @@ -130,6 +139,7 @@ describe('Document', () => {
let findHandler: any;
let anchorService: AnchorService;
let ceramic: Ceramic;
let ceramicWithoutSchemaValidation: Ceramic;
let context: Context;

beforeEach(() => {
Expand Down Expand Up @@ -177,6 +187,9 @@ describe('Document', () => {

ceramic = new Ceramic(dispatcher, pinStore, context)
ceramic._doctypeHandlers['tile'] = doctypeHandler

ceramicWithoutSchemaValidation = new Ceramic(dispatcher, pinStore, context, false)
ceramicWithoutSchemaValidation._doctypeHandlers['tile'] = doctypeHandler
})

it('is created correctly', async () => {
Expand Down Expand Up @@ -326,6 +339,66 @@ describe('Document', () => {
await doc1._handleTip(tipInvalidUpdate)
expect(doc1.content).toEqual(newContent)
})

it('Enforces schema at document creation', async () => {
const schemaDoc = await create({ content: stringMapSchema, metadata: { controllers } }, ceramic, context)
await anchorUpdate(schemaDoc)

try {
const docParams = {
content: {stuff: 1},
metadata: {controllers, schema: schemaDoc.id.toString()}
}
await create(docParams, ceramic, context)
throw new Error('Should not be able to create a document with an invalid schema')
} catch (e) {
expect(e.message).toEqual('Validation Error: data[\'stuff\'] should be string')
}
})

it('Enforces schema at document update', async () => {
const schemaDoc = await create({ content: stringMapSchema, metadata: { controllers } }, ceramic, context)
await anchorUpdate(schemaDoc)

const docParams = {
content: {stuff: 1},
metadata: {controllers}
}
const doc = await create(docParams, ceramic, context)
await anchorUpdate(doc)

try {
const updateRec = await TileDoctype._makeRecord(doc.doctype, user, null, doc.controllers, schemaDoc.id.toString())
await doc.applyRecord(updateRec)
throw new Error('Should not be able to assign a schema to a document that does not conform')
} catch (e) {
expect(e.message).toEqual('Validation Error: data[\'stuff\'] should be string')
}
})

it('Enforces schema when loading genesis record', async () => {
const schemaDoc = await create({ content: stringMapSchema, metadata: { controllers } }, ceramic, context)
await anchorUpdate(schemaDoc)

const docParams = {
content: {stuff: 1},
metadata: {controllers, schema: schemaDoc.id.toString()}
}
// Create a document that isn't conforming to the schema
const doc = await create(docParams, ceramicWithoutSchemaValidation, context)
await anchorUpdate(doc)

expect(doc.content).toEqual({stuff:1})
expect(doc.metadata.schema).toEqual(schemaDoc.id.toString())

try {
await Document.load(doc.id, findHandler, dispatcher, pinStore, context, {skipWait:true})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual test for the core fix in this PR. Without the change to Document.load, this test fails.

throw new Error('Should not be able to assign a schema to a document that does not conform')
} catch (e) {
expect(e.message).toEqual('Validation Error: data[\'stuff\'] should be string')
}
})

})

describe('Network update logic', () => {
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ class Document extends EventEmitter {
doc._doctype.state = await doc._doctypeHandler.applyRecord(record, doc._genesisCid, context)
}

if (validate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, that's it 👍

const schema = await Document.loadSchema(context.api, doc._doctype)
if (schema) {
Utils.validate(doc._doctype.content, schema)
}
}

await doc._register(opts)
return doc
}
Expand Down