Skip to content

graph, store, graphql: Add entityCount field to subgraph entities - #602

Merged
leoyvens merged 1 commit into
masterfrom
leo/number-of-entities
Nov 28, 2018
Merged

leoyvens merged 1 commit into
masterfrom
leo/number-of-entities

Conversation

@leoyvens

Copy link
Copy Markdown
Contributor

Resolves #583 (the url should be tracked in a different issue).

This adds the entityCount field. Instead of storing it with the data and incrementing on every entity insert, which would be error-prone, this introduces the concept of a "calculated field", which is a field of an entity that is not in the store but rather calculated on the fly. We can probably also use this for __typename.

}

/// Compute special fields that are not stored such as as `entityCount`.
fn add_computed_fields(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall, this is great, but I would do this part differently. Adding computed fields like this will mean that we'll always count entities for all subgraphs, even if entityCount is not part of the query.

What I would do instead is resolve fields like this lazily. Here's what I'd recommend:

  1. Change the signature of Resolver::resolve_scalar_value so that it takes the object type and the field name:
    fn resolve_scalar_value(
        &self,
        object_type: &s::ObjectType,
        field_name: &q::Name,
        scalar_type: &s::ScalarType,
        value: Option<&q::Value>,
    ) -> q::Value {
  2. Implement resolve_scalar_value in StoreResolver. Something like this should do:
     fn resolve_scalar_value(
         &self,	
         object_type: &s::ObjectType,
         field_name: &q::Name,
         scalar_type: &s::ScalarType,
         value: Option<&q::Value>,
     ) -> q::Value {
         match field_name.as_str() {
           "entityCount" => {
             ... parse subgraph ID, if possible, fall back to value ...
             ... obtain entity count ...
           },
           _ => value
         }.and_then(|value| value.coerce(scalar_type)).unwrap_or(q::Value::Null)
     }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But I'm not completely convinced of my approach because it makes adding support for GraphQL interfaces harder (which don't have ObjectType). Perhaps let's go with your approach first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Overall, this is great, but I would do this part differently. Adding computed fields like this will mean that we'll always count entities for all subgraphs

I hadn't thought about this. I'll follow your recommendation then and keep things as they are for now, we can revisit the general issue of overfetching things later.

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.

2 participants