Skip to content

Commit

Permalink
Exclude updatable queries from operation transforms pipeline
Browse files Browse the repository at this point in the history
Summary:
Updatable queries do not get sent to the server. Consider usePrivacyIncidentFactGatheringContentStateQuery.graphql.js. The generated artifact only includes fragment and kind fields, i.e. no operation and no metadata. As a consequence, all of the work that we do to updatable queries in the `apply_operation_transforms`, `apply_normalization_transforms`, and `apply_operation_text_transforms` pipelines in apply_transforms.rs:

* is useless, since the results aren't used
* are actively harmful, in that the transforms in this pipeline enforce certain invariants that make no sense. e.g. updatable queries with only client only fields are disallowed — but that is in fact fine!

This change modifies the underpinnings to properly skip client-side-only `updatable` Queries.  The core problem of erroring out remains, since other logic causes empty queries to panic.

Reviewed By: rbalicki2

Differential Revision: D38132333

fbshipit-source-id: f1dea8f449ac9d5d289edaadcbdad18e1ee27987
  • Loading branch information
Russ West authored and facebook-github-bot committed Aug 2, 2022
1 parent 168a9fc commit 44ecca8
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,6 @@ pub fn generate_artifacts(
source_hash,
source_file,
)
} else if normalization.directives.named(*UPDATABLE_DIRECTIVE).is_some() {
// This weird structuring here is to preserve the order`
// of operations/fallbacks in the face of an "Optional" normalizaiton
// Right now, normalization will still be there, so we need this branch
// in a coming commit we will no longer generate the normalization AST for
// updatable queries and the "dangling if" below will become an "else if"
} else {
let source_hash = source_hashes
.get(&normalization.name.item)
Expand All @@ -131,9 +125,7 @@ pub fn generate_artifacts(
normalization.name.location.source_location(),
)
}
}

if let Some(reader) = operations.reader {
} else if let Some(reader) = operations.reader {
// We don't have a normalization AST, but we do have a reader.
// Therefore this must be an updatable query in order to continue.
if reader
Expand Down
5 changes: 5 additions & 0 deletions compiler/crates/relay-transforms/src/apply_transforms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::assignable_fragment_spread::replace_updatable_fragment_spreads;
use crate::client_extensions_abstract_types::client_extensions_abstract_types;
use crate::disallow_non_node_id_fields;
use crate::match_::hash_supported_argument;
use crate::skip_updatable_queries::skip_updatable_queries;
use common::sync::try_join;
use common::DiagnosticsResult;
use common::PerfLogEvent;
Expand Down Expand Up @@ -308,6 +309,10 @@ fn apply_operation_transforms(
None,
)?;

program = log_event.time("skip_updatable_queries", || {
skip_updatable_queries(&program)
});

program = log_event.time("client_edges", || {
client_edges(&program, &project_config.schema_config)
})?;
Expand Down
1 change: 1 addition & 0 deletions compiler/crates/relay-transforms/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ mod skip_null_arguments_transform;
mod skip_redundant_nodes;
mod skip_split_operation;
mod skip_unreachable_node;
mod skip_updatable_queries;
mod sort_selections;
mod test_operation_metadata;
mod transform_connections;
Expand Down
52 changes: 52 additions & 0 deletions compiler/crates/relay-transforms/src/skip_updatable_queries.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

use crate::UPDATABLE_DIRECTIVE;
use common::Named;
use graphql_ir::OperationDefinition;
use graphql_ir::Program;
use graphql_ir::Transformed;
use graphql_ir::Transformer;

pub fn skip_updatable_queries(program: &Program) -> Program {
let mut transform = SkipUpdatableQueries::new(program);
let transformed = transform.transform_program(program);

transformed.replace_or_else(|| program.clone())
}

#[allow(dead_code)]
struct SkipUpdatableQueries<'s> {
program: &'s Program,
}

impl<'s> SkipUpdatableQueries<'s> {
fn new(program: &'s Program) -> Self {
Self { program }
}
}

impl<'s> Transformer for SkipUpdatableQueries<'s> {
const NAME: &'static str = "SkipUpdatableQueriesTransform";
const VISIT_ARGUMENTS: bool = false;
const VISIT_DIRECTIVES: bool = true;

fn transform_operation(
&mut self,
operation: &OperationDefinition,
) -> Transformed<OperationDefinition> {
if operation
.directives
.iter()
.any(|directive| directive.name() == *UPDATABLE_DIRECTIVE)
{
Transformed::Delete
} else {
Transformed::Keep
}
}
}

0 comments on commit 44ecca8

Please sign in to comment.