Skip to content

Commit

Permalink
Fix compiler issue with stale artifacts for Resolvers in watch mode (#…
Browse files Browse the repository at this point in the history
…4415)

Summary:
The Issue:
When running the compiler in watch mode, the compiler may produce
artifacts for relay resolvers.

For example:
```
/** RelayResolver MyType */
```
This code would create fragments for the model resolver, including
an identifier, a model instance fragment, etc.

However, a problem arises in watch mode when we rename the type to:

```
/** RelayResolver  MyNewType */
```

In this case, the old artifacts are not automatically deleted. The issue lies in
the compiler's inability to establish a connection between the resolver
definition code and the artifacts it produces.

To resolve this, we need to establish that connection.

The solution:

The relationship between the source documents (fragments, queries, and now resolvers)
and the generated artifacts is represented in the artifact_map. In this map, the key
corresponds to the document's name (fragment or query), and the value is a
list of the generated artifacts.

For resolvers, we will use the resolver's hash (MD5 of the source) as the key
in the map, and the associated value will be the list of generated artifacts.

The solution involves passing this hash to the artifact generation *place*
and ensuring the proper handling of the removal of outdated artifacts.

Pull Request resolved: #4415

Test Plan:
**Watch mode**

- Add new resolver /** RelayResolver MyType **/
- New files are created for the resolver (id, model_instance)
- Change the name to  /** RelayResolver MyNewType **/
- New files are created, files for MyType are deleted.

 ---

**Non-watch Compilation (no saved state)**

Run 1:
- Add new resolver /** RelayResolver MyType **/
- New files are created for the resolver (id, model_instance)

Rename - Change the name to  /** RelayResolver MyNewType **/

Run 2:
- New files are created, files for MyType are deleted.

 ---

**Non-watch Compilation (with saved-state)**

Pre-run:

- Add new resolver /** RelayResolver MyType **/
- New files are created for the resolver (id, model_instance)
- Commit changes.

Run compiler and create saved state:

```
./scripts/compiler.sh xplat build --export-state=/tmp/compiler-state
```

Run compiler with saved state

```
./scripts/compiler.sh xplat build --import-state=/tmp/compiler-state
```

- No changes, quick compilation.
- Update file: Change the name to  /** RelayResolver MyNewType **/

Run compiler with the saved state:

Old files deleted, new files created:
 {F1076577864}

 ---

**Build with saved-state + dirty resolvers artifact**

See the same steps for for preparation of the saved-state.

Also run:

- Update/Remove generated artifact for the Resolver
- Run compiler with saved-state
- Artifacts are restored to the correct version.

Reviewed By: captbaritone

Differential Revision: D48588439

Pulled By: alunyov

fbshipit-source-id: 72fbebbf79eed0d512d24575601b203c3aee85a1
  • Loading branch information
alunyov authored and facebook-github-bot committed Aug 29, 2023
1 parent 7945157 commit 15c2715
Show file tree
Hide file tree
Showing 68 changed files with 477 additions and 132 deletions.
5 changes: 5 additions & 0 deletions compiler/Cargo.lock

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

3 changes: 3 additions & 0 deletions compiler/crates/docblock-shared/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,8 @@ license = "MIT"

[dependencies]
common = { path = "../common" }
hex = "0.4.3"
intern = { path = "../intern" }
lazy_static = "1.4"
md-5 = "0.10"
serde = { version = "1.0.185", features = ["derive", "rc"] }
7 changes: 7 additions & 0 deletions compiler/crates/docblock-shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
* LICENSE file in the root directory of this source tree.
*/

mod resolver_source_hash;

use common::ArgumentName;
use common::DirectiveName;
use common::ScalarName;
use intern::string_key::Intern;
use intern::string_key::StringKey;
use lazy_static::lazy_static;
pub use resolver_source_hash::ResolverSourceHash;

lazy_static! {
pub static ref RELAY_RESOLVER_DIRECTIVE_NAME: DirectiveName =
Expand Down Expand Up @@ -51,4 +54,8 @@ lazy_static! {
// Note: this should **only** be used for resolvers! The id field for server
// types is configurable in the config, and thus cannot be hard-coded.
pub static ref KEY_RESOLVER_ID_FIELD: StringKey = "id".intern();

pub static ref RELAY_RESOLVER_SOURCE_HASH: DirectiveName = DirectiveName("resolver_source_hash".intern());
pub static ref RELAY_RESOLVER_SOURCE_HASH_VALUE: ArgumentName = ArgumentName("value".intern());

}
37 changes: 37 additions & 0 deletions compiler/crates/docblock-shared/src/resolver_source_hash.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* 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 intern::string_key::Intern;
use intern::string_key::StringKey;
use md5::Digest;
use md5::Md5;
use serde::Deserialize;
use serde::Serialize;

#[derive(Serialize, Deserialize, Debug, Clone, Copy, Eq, PartialEq, Hash)]

pub struct ResolverSourceHash(StringKey);

impl ResolverSourceHash {
pub fn new(source: &str) -> Self {
Self(md5(source).intern())
}

pub fn from_raw(source: StringKey) -> Self {
Self(source)
}

pub fn value(&self) -> StringKey {
self.0
}
}

fn md5(data: &str) -> String {
let mut md5 = Md5::new();
md5.update(data);
hex::encode(md5.finalize())
}
1 change: 1 addition & 0 deletions compiler/crates/docblock-syntax/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ path = "tests/parse_test.rs"

[dependencies]
common = { path = "../common" }
docblock-shared = { path = "../docblock-shared" }
intern = { path = "../intern" }
serde = { version = "1.0.185", features = ["derive", "rc"] }
thiserror = "1.0.43"
Expand Down
2 changes: 2 additions & 0 deletions compiler/crates/docblock-syntax/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use common::Location;
use common::Named;
use common::WithLocation;
use docblock_shared::ResolverSourceHash;
use intern::string_key::StringKey;
#[derive(Debug, PartialEq)]
pub struct DocblockField {
Expand All @@ -32,6 +33,7 @@ pub enum DocblockSection {
pub struct DocblockAST {
pub location: Location,
pub sections: Vec<DocblockSection>,
pub source_hash: ResolverSourceHash,
}

impl DocblockAST {
Expand Down
4 changes: 4 additions & 0 deletions compiler/crates/docblock-syntax/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use common::SourceLocationKey;
use common::Span;
use common::TextSource;
use common::WithLocation;
use docblock_shared::ResolverSourceHash;
use errors::SyntaxError;
use intern::string_key::Intern;
use intern::string_key::StringKey;
Expand Down Expand Up @@ -94,6 +95,7 @@ struct DocblockParser<'a> {
errors: Vec<Diagnostic>,
in_progress_text: Option<SpanString>,
sections: Vec<DocblockSection>,
source_hash: ResolverSourceHash,
}

impl<'a> DocblockParser<'a> {
Expand All @@ -106,6 +108,7 @@ impl<'a> DocblockParser<'a> {
chars,
in_progress_text: None,
sections: Vec::new(),
source_hash: ResolverSourceHash::new(source),
}
}

Expand Down Expand Up @@ -146,6 +149,7 @@ impl<'a> DocblockParser<'a> {
*/
Span::new(start, end - 1),
),
source_hash: self.source_hash,
})
} else {
Err(self.errors)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,7 @@ DocblockAST {
},
),
],
source_hash: ResolverSourceHash(
"4595a2a06568991a6d7594afbbe370ab",
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,7 @@
DocblockAST {
location: empty-block.ecmascript:0:2,
sections: [],
source_hash: ResolverSourceHash(
"75966d011d530b35f16483422368e364",
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ DocblockAST {
},
),
],
source_hash: ResolverSourceHash(
"b408df07614b47f8b522bf4e528448d3",
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,7 @@ DocblockAST {
},
),
],
source_hash: ResolverSourceHash(
"a49d092bbb5fbad6582962e62fac5d2c",
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,7 @@ DocblockAST {
},
),
],
source_hash: ResolverSourceHash(
"c8f9872da22c679b3a9385d453c2d7d8",
),
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,7 @@ DocblockAST {
},
),
],
source_hash: ResolverSourceHash(
"39bf0ba989cb8ca55688669aa89f1f43",
),
}
6 changes: 3 additions & 3 deletions compiler/crates/graphql-ir/src/program.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl Program {
pub fn merge_program(
&mut self,
other_program: &Self,
removed_definition_names: Option<&[ExecutableDefinitionName]>,
removed_definition_names: Option<Vec<ExecutableDefinitionName>>,
) {
let mut operations: HashMap<
OperationDefinitionName,
Expand All @@ -147,10 +147,10 @@ impl Program {
for removed in removed_definition_names {
match removed {
ExecutableDefinitionName::OperationDefinitionName(name) => {
operations.remove(name);
operations.remove(&name);
}
ExecutableDefinitionName::FragmentDefinitionName(name) => {
self.fragments.remove(name);
self.fragments.remove(&name);
}
};
}
Expand Down
1 change: 1 addition & 0 deletions compiler/crates/relay-compiler/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ common = { path = "../common" }
common-path = "1.0.0"
dashmap = { version = "5.4", features = ["rayon", "serde"] }
dependency-analyzer = { path = "../dependency-analyzer" }
docblock-shared = { path = "../docblock-shared" }
docblock-syntax = { path = "../docblock-syntax" }
dunce = "1.0.2"
errors = { path = "../errors" }
Expand Down
18 changes: 16 additions & 2 deletions compiler/crates/relay-compiler/src/artifact_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ use std::path::PathBuf;

use dashmap::mapref::entry::Entry;
use dashmap::DashMap;
use docblock_shared::ResolverSourceHash;
use graphql_ir::ExecutableDefinitionName;
use relay_codegen::QueryID;
use relay_transforms::ArtifactSourceKeyData;
use serde::Deserialize;
use serde::Serialize;

Expand All @@ -25,7 +27,19 @@ pub struct ArtifactRecord {
}
/// A map from DefinitionName to output artifacts records
#[derive(Default, Serialize, Deserialize, Debug, Clone)]
pub struct ArtifactMap(pub DashMap<ExecutableDefinitionName, Vec<ArtifactRecord>>);
pub struct ArtifactMap(pub DashMap<ArtifactSourceKey, Vec<ArtifactRecord>>);

#[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq, Hash)]
pub enum ArtifactSourceKey {
ExecutableDefinition(ExecutableDefinitionName),
ResolverHash(ResolverSourceHash),
}

impl From<ArtifactSourceKeyData> for ArtifactSourceKey {
fn from(directive: ArtifactSourceKeyData) -> Self {
ArtifactSourceKey::ResolverHash(directive.0)
}
}

impl ArtifactMap {
pub fn insert(&self, artifact: Artifact) {
Expand All @@ -42,7 +56,7 @@ impl ArtifactMap {
},
};

for source_definition_name in artifact.source_definition_names {
for source_definition_name in artifact.artifact_source_keys {
match self.0.entry(source_definition_name) {
Entry::Occupied(mut entry) => {
entry.get_mut().push(artifact_tuple.clone());
Expand Down
Loading

0 comments on commit 15c2715

Please sign in to comment.