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

Add new API Version to validate when setting fields not defined in the schema #4894

Merged
merged 7 commits into from
Oct 8, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
24 changes: 6 additions & 18 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion graph/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ bytes = "1.0.1"
cid = "0.10.1"
diesel = { version = "1.4.8", features = ["postgres", "serde_json", "numeric", "r2d2", "chrono"] }
diesel_derives = "1.4"
chrono = "0.4.25"
chrono = "0.4.31"
envconfig = "0.10.0"
Inflector = "0.11.3"
isatty = "0.1.9"
Expand Down
11 changes: 0 additions & 11 deletions graph/src/data/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,6 @@ impl<E, T: IntoIterator<Item = Result<(Word, Value), E>>> TryIntoEntityIterator<

#[derive(Debug, Error, PartialEq, Eq, Clone)]
pub enum EntityValidationError {
#[error("The provided entity has fields not defined in the schema for entity `{entity}`")]
FieldsNotDefined { entity: String },

#[error("Entity {entity}[{id}]: unknown entity type `{entity}`")]
UnknownEntityType { entity: String, id: String },

Expand Down Expand Up @@ -928,14 +925,6 @@ impl Entity {
}
})?;

for field in self.0.atoms() {
if !schema.has_field(&key.entity_type, field) {
return Err(EntityValidationError::FieldsNotDefined {
entity: key.entity_type.clone().into_string(),
});
}
}

for field in &object_type.fields {
let is_derived = field.is_derived();
match (self.get(&field.name), is_derived) {
Expand Down
3 changes: 3 additions & 0 deletions graph/src/data/subgraph/api_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ pub const API_VERSION_0_0_6: Version = Version::new(0, 0, 6);
/// Enables event handlers to require transaction receipts in the runtime.
pub const API_VERSION_0_0_7: Version = Version::new(0, 0, 7);

/// Enables validation for fields that doesnt exist in the schema for an entity.
Copy link
Collaborator

Choose a reason for hiding this comment

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

small typo: s/doesnt/don't/

pub const API_VERSION_0_0_8: Version = Version::new(0, 0, 8);

/// Before this check was introduced, there were already subgraphs in the wild with spec version
/// 0.0.3, due to confusion with the api version. To avoid breaking those, we accept 0.0.3 though it
/// doesn't exist.
Expand Down
4 changes: 2 additions & 2 deletions graph/src/env/mappings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub struct EnvVarsMapping {
/// kilobytes). The default value is 10 megabytes.
pub entity_cache_size: usize,
/// Set by the environment variable `GRAPH_MAX_API_VERSION`. The default
/// value is `0.0.7`.
/// value is `0.0.8`.
pub max_api_version: Version,
/// Set by the environment variable `GRAPH_MAPPING_HANDLER_TIMEOUT`
/// (expressed in seconds). No default is provided.
Expand Down Expand Up @@ -93,7 +93,7 @@ pub struct InnerMappingHandlers {
entity_cache_dead_weight: EnvVarBoolean,
#[envconfig(from = "GRAPH_ENTITY_CACHE_SIZE", default = "10000")]
entity_cache_size_in_kb: usize,
#[envconfig(from = "GRAPH_MAX_API_VERSION", default = "0.0.7")]
#[envconfig(from = "GRAPH_MAX_API_VERSION", default = "0.0.8")]
max_api_version: Version,
#[envconfig(from = "GRAPH_MAPPING_HANDLER_TIMEOUT")]
mapping_handler_timeout_in_secs: Option<u64>,
Expand Down
14 changes: 14 additions & 0 deletions graph/src/schema/input_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,20 @@ impl InputSchema {
.map(|fields| fields.contains(&field))
.unwrap_or(false)
}

pub fn has_field_with_name(&self, entity_type: &EntityType, field: &str) -> bool {
let field = self.inner.pool.lookup(field);

match field {
Some(field) => self
.inner
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just call self.has_field at this point; avoids duplicating logic

.field_names
.get(entity_type)
.map(|fields| fields.contains(&field))
.unwrap_or(false),
None => false,
}
}
}

/// Create a new pool that contains the names of all the types defined
Expand Down
50 changes: 41 additions & 9 deletions runtime/test/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1225,8 +1225,13 @@ struct Host {
}

impl Host {
async fn new(schema: &str, deployment_hash: &str, wasm_file: &str) -> Host {
let version = ENV_VARS.mappings.max_api_version.clone();
async fn new(
schema: &str,
deployment_hash: &str,
wasm_file: &str,
api_version: Option<Version>,
) -> Host {
let version = api_version.unwrap_or(ENV_VARS.mappings.max_api_version.clone());
let wasm_file = wasm_file_path(wasm_file, API_VERSION_0_0_5);

let ds = mock_data_source(&wasm_file, version.clone());
Expand Down Expand Up @@ -1324,7 +1329,7 @@ async fn test_store_set_id() {
name: String,
}";

let mut host = Host::new(schema, "hostStoreSetId", "boolean.wasm").await;
let mut host = Host::new(schema, "hostStoreSetId", "boolean.wasm", None).await;

host.store_set(USER, UID, vec![("id", "u1"), ("name", "user1")])
.expect("setting with same id works");
Expand Down Expand Up @@ -1414,7 +1419,13 @@ async fn test_store_set_invalid_fields() {
test2: String
}";

let mut host = Host::new(schema, "hostStoreSetInvalidFields", "boolean.wasm").await;
let mut host = Host::new(
schema,
"hostStoreSetInvalidFields",
"boolean.wasm",
Some(API_VERSION_0_0_8),
)
.await;

host.store_set(USER, UID, vec![("id", "u1"), ("name", "user1")])
.unwrap();
Expand All @@ -1437,8 +1448,7 @@ async fn test_store_set_invalid_fields() {
// So we just check the string contains them
let err_string = err.to_string();
dbg!(err_string.as_str());
assert!(err_string
.contains("The provided entity has fields not defined in the schema for entity `User`"));
assert!(err_string.contains("Entity `User` has fields not in schema: test2, test"));

let err = host
.store_set(
Expand All @@ -1449,8 +1459,30 @@ async fn test_store_set_invalid_fields() {
.err()
.unwrap();

err_says(
err,
"Unknown key `test3`. It probably is not part of the schema",
err_says(err, "Entity `User` has fields not in schema: test3");

// For apiVersion below 0.0.8, we should not error out
let mut host2 = Host::new(
schema,
"hostStoreSetInvalidFields",
"boolean.wasm",
Some(API_VERSION_0_0_7),
)
.await;

let err_is_none = host2
.store_set(
USER,
UID,
vec![
("id", "u1"),
("name", "user1"),
("test", "invalid_field"),
("test2", "invalid_field"),
],
)
.err()
.is_none();

assert!(err_is_none);
}
51 changes: 50 additions & 1 deletion runtime/wasm/src/host_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::ops::Deref;
use std::str::FromStr;
use std::time::{Duration, Instant};

use graph::data::subgraph::API_VERSION_0_0_8;
use graph::data::value::Word;

use never::Never;
Expand Down Expand Up @@ -199,9 +200,57 @@ impl<C: Blockchain> HostExports<C> {
}
}

// From apiVersion 0.0.8 onwards, we check that the entity data
// does not contain fields that are not in the schema and fail
if self.api_version >= API_VERSION_0_0_8 {
// Quick check for any invalid field
let has_invalid_fields = data.iter().any(|(field_name, _)| {
!state
.entity_cache
.schema
.has_field_with_name(&key.entity_type, &field_name)
});

// If an invalid field exists, find all and return an error
if has_invalid_fields {
let invalid_fields: Vec<Word> = data
.iter()
.filter_map(|(field_name, _)| {
if !state
.entity_cache
.schema
.has_field_with_name(&key.entity_type, &field_name)
{
Some(field_name.clone())
} else {
None
}
})
.collect();

return Err(HostExportError::Deterministic(anyhow!(
"Attempted to set undefined fields [{}] for the entity type `{}`. Make sure those fields are defined in the schema.",
invalid_fields
.iter()
.map(|f| f.as_str())
.collect::<Vec<_>>()
.join(", "),
key.entity_type
)));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the block starting at line 203 be put into a check_invalid_fields helper function? It would make store_set more readable.


// Filter out fields that are not in the schema
let filtered_entity_data = data.into_iter().filter(|(field_name, _)| {
state
.entity_cache
.schema
.has_field_with_name(&key.entity_type, field_name)
});

let entity = state
.entity_cache
.make_entity(data.into_iter().map(|(key, value)| (key, value)))
.make_entity(filtered_entity_data.map(|(key, value)| (key, value)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

That map is a noop (was one before, too) and can just be removed

.map_err(|e| HostExportError::Deterministic(anyhow!(e)))?;

let poi_section = stopwatch.start_section("host_export_store_set__proof_of_indexing");
Expand Down
15 changes: 15 additions & 0 deletions tests/runner-tests/api-version/abis/Contract.abi
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[
{
"anonymous": false,
"inputs": [
{
"indexed": false,
"internalType": "string",
"name": "testCommand",
"type": "string"
}
],
"name": "TestEvent",
"type": "event"
}
]
3 changes: 3 additions & 0 deletions tests/runner-tests/api-version/data.0.0.7.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"apiVersion": "0.0.7"
}
3 changes: 3 additions & 0 deletions tests/runner-tests/api-version/data.0.0.8.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"apiVersion": "0.0.8"
}
29 changes: 29 additions & 0 deletions tests/runner-tests/api-version/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"name": "api-version",
"version": "0.1.0",
"scripts": {
"build-contracts": "../../common/build-contracts.sh",
"codegen": "graph codegen --skip-migrations",
"test": "yarn build-contracts && truffle test --compile-none --network test",
"create:test": "graph create test/api-version --node $GRAPH_NODE_ADMIN_URI",
"prepare:0-0-7": "mustache data.0.0.7.json subgraph.template.yaml > subgraph.yaml",
"prepare:0-0-8": "mustache data.0.0.8.json subgraph.template.yaml > subgraph.yaml",
"deploy:test-0-0-7": "yarn prepare:0-0-7 && graph deploy test/api-version-0-0-7 --version-label 0.0.7 --ipfs $IPFS_URI --node $GRAPH_NODE_ADMIN_URI",
"deploy:test-0-0-8": "yarn prepare:0-0-8 && graph deploy test/api-version-0-0-8 --version-label 0.0.8 --ipfs $IPFS_URI --node $GRAPH_NODE_ADMIN_URI"
},
"devDependencies": {
"@graphprotocol/graph-cli": "0.53.0",
"@graphprotocol/graph-ts": "0.31.0",
"solc": "^0.8.2"
},
"dependencies": {
"@truffle/contract": "^4.3",
"@truffle/hdwallet-provider": "^1.2",
"apollo-fetch": "^0.7.0",
"babel-polyfill": "^6.26.0",
"babel-register": "^6.26.0",
"gluegun": "^4.6.1",
"mustache": "^4.2.0",
"truffle": "^5.2"
}
}
4 changes: 4 additions & 0 deletions tests/runner-tests/api-version/schema.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
type TestResult @entity {
id: ID!
message: String!
}
15 changes: 15 additions & 0 deletions tests/runner-tests/api-version/src/mapping.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Entity, Value, store } from "@graphprotocol/graph-ts";
import { TestEvent } from "../generated/Contract/Contract";
import { TestResult } from "../generated/schema";

export function handleTestEvent(event: TestEvent): void {
let testResult = new TestResult(event.params.testCommand);
testResult.message = event.params.testCommand;
let testResultEntity = testResult as Entity;
testResultEntity.set(
"invalid_field",
Value.fromString("This is an invalid field"),
);
store.set("TestResult", testResult.id, testResult);
testResult.save();
}
23 changes: 23 additions & 0 deletions tests/runner-tests/api-version/subgraph.template.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
specVersion: 0.0.4
schema:
file: ./schema.graphql
dataSources:
- kind: ethereum/contract
name: Contract
network: test
source:
address: "0x0000000000000000000000000000000000000000"
abi: Contract
mapping:
kind: ethereum/events
apiVersion: {{apiVersion}}
language: wasm/assemblyscript
abis:
- name: Contract
file: ./abis/Contract.abi
entities:
- Call
eventHandlers:
- event: TestEvent(string)
handler: handleTestEvent
file: ./src/mapping.ts
Loading