Rust implementation of the core spec #326

queerviolet · 2021-01-14T16:02:18Z

Define a new crate, core-schema, which provides Rust support for the current draft @core directive.

The intention here is to provide (1) a crate we can use in the query planner to process composed schema directives (2) a reference implementation of the spec which can be ported to other languages if needed.

DONE:

TODO:

an implementation activator which lets you register implementations by (spec identity, spec version) and iterate over matches of requested implementations
tests for url parsing / version resolution
micro-cli devtool for extracting using information from a schema
better docs and examples

… to root.

queerviolet · 2021-01-14T16:35:37Z

sorry about the file movement noise y'all. If you want to hide all the moved files in this PR, you can run this snippet in the dev console:

document.querySelectorAll('a[title^="stargate"]').forEach(e => e.closest('.file').remove())

(there are moved files because I put graphql-parser and query-planner back in the root of the repo, since we're using them both outside of stargate)

… nightly only.

queerviolet · 2021-01-14T23:27:00Z

This is ready for review

abernix · 2021-01-15T16:45:57Z

sorry about the file movement noise y'all. If you want to hide all the moved files in this PR, you can run this snippet in the dev console:
document.querySelectorAll('a[title^="stargate"]').forEach(e => e.closest('.file').remove())
(there are moved files because I put graphql-parser and query-planner back in the root of the repo, since we're using them both outside of stargate)

@queerviolet Since we already agreed to hoist the ./stargate-nested crates back to the top of the repository, I think the file move stuff should/could survive on its own and would benefit from being de-coupled from this PR and getting landed expeditiously, ensure that it gets its own separate commit for posterity, and still forgo the need for Console filter ergonomics. (The burden in review is quite different between the two initiatives.)

Could we cherry pick I think, just 044a6bf (#326) into its own branch and PR and land that separately? (This PR could be re-based/targeted against that branch in the interim. They'd automatically waterfall merge into main.)

… and create the Bounds trait to get bounds if desired.

lrlna

Hello!

I had a chance to look through this today finally. I hope some of these comments are helpful. Please let me know if I can clarify anything, or you wanted more specific feedback anywhere in this code!

One overarching comment I have is to run this with cargo clippy, as there are a few warnings it's currently printing for this crate (and I can't review it better than clippy for those parts anyways)

lrlna · 2021-01-19T14:29:24Z

using/src/schema.rs

+    /// displaying the error.
+    ///
+    /// Returns the empty string if this error does not reference any `Request`s.
+    fn requests(&self) -> String {


I would implement a Display for SchemaError in this case, something like this:

impl fmt::Display for SchemaError { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { let formatting_logic = // what you have already written below in your code write!(fmt, "{}", formatting_logic) } }

well, derive(thiserror::Error) is taking care of implementing fmt::Display. We're using that for errors everywhere else, so I kindof wanted to maintain that here (it also looks nice). But specifically for the OverlappingPrefix case, I wanted to list all the overlapping requests as part of the error, which requires a bit more implementation than I wanted to put into the #[error(...)] attribute, so we call out to this fn

lrlna · 2021-01-19T14:33:14Z

using/src/version.rs

+        }
+    }
+
+    /// Return true iff this Version satisfies the `required` version


smol typo

Suggested change

/// Return true iff this Version satisfies the `required` version

/// Return true if this Version satisfies the `required` version

oh, that's intentional, "iff" = "if and only if"

but i can spell that out

ohhhhh definitely didn't know that! ++ on spelling it out then.

lrlna · 2021-01-19T14:47:19Z

using/src/version.rs

+use regex::Regex;
+use thiserror::Error;
+
+/// Versions are a (major, minor) u64 pair.


I guess this can't use the semver crate because graphql versions are not technically semver?

Yeah, we don't have patch versions. I thought about maybe hacking it (appending a patch version of 0 always), but the logic is simple enough that I'm not sure it's worth it.

ah yea! this approach seems good then (i think semver implementation is pretty much what you have here, plus the extra five lines for minor anyways)

lrlna · 2021-01-19T15:09:28Z

using/src/schema.rs

+        Ok(machined)
+    }
+
+    /// Validate that no two `Request`s are using the same `prefix`, removing


You do not need a doc comment here (///) since this is not a public function (just // works)

lrlna · 2021-01-19T15:10:08Z

using/src/schema.rs

+    }
+}
+
+/// Drain all items from a `Vec<T>` which match `pred` and collect the results


does not need to be a doc comment

lrlna · 2021-01-19T18:00:47Z

using/src/activations.rs

+/// matching implementations were available.
+// type Activation<'schema, 'impls, T> = (&'schema Request, Find<'impls, T>);
+
+impl<'a> Schema<'a> {


What is the reason for having this not part of schema.rs? It's definitely not standard, and from what I understand of this PR I don't see a reason for it to be separate, but maybe I am missing something!

lrlna · 2021-01-19T18:03:09Z

using/src/activations.rs

+    /// # Ok::<(), GraphQLParseError>(())
+    /// ```
+    pub fn activations<'impls, T>(
+        &'impls self,


With impl being a reserved word, it's a bit weird to have 'impls as a name for a lifetime. Would definitely use something else (I do understand it's trying to describe a lifetime of Implementations, which is just very close to impl hah)

using/src/request.rs

lrlna · 2021-01-19T18:14:16Z

using/src/request.rs

+
+use crate::spec::{Spec, SpecParseError};
+
+/// Requests contain a `spec`, the `prefix` requested for that spec (which


nit: for doc comments, it's usually really nice to have the first line to be a bit shorter and then an added break. When the docs get built the first line is used as a description for that particular exported type. This is what it looks like now for Request vs Pos

lrlna · 2021-01-19T18:17:55Z

using/src/schema.rs

@@ -0,0 +1,693 @@
+//! A GraphQL schema referencing one or more specifications


I would either move this to right above pub struct Schema<'a>, so that the docs built directly for the Schema struct, or, add another set of comments just above there, since this page is currently empty:

queerviolet · 2021-01-27T22:46:45Z

PTAL @lrlna @trevor-scheer

@lrlna, great cow suggestion, I've implemented your changes. As I've been working with this library in #305, I've found that it would indeed be very helpful to be able to easily define specs as top-level consts. This is really nice!

lrlna · 2021-02-01T17:59:28Z

@queerviolet in regards to your first question:

Another approach would be to simply have a fn default_using() -> Request which always allocs a new Request with the default using spec parameters. I'd expect the performance hit to be effectively 0 (we already do a lot more allocations when parsing even a simple schema). Though admittedly, the cognitive cost of Cow vs String is quite low too. Thoughts?

My answer here depends on how often you expect to use Request and Spec directly. It's for sure very tidy consts exported for other crate users, and I will take Cow implementation over a lazy_static. However, if you need to instantiate a default Request and Spec pretty frequently, and it does not need to be a const, I would consider writing a Default impl. That way not only do you avoid a lazy_static, you can also keep the Request implementation to still accept String, which is a lot more ergonomic for the users of the library. They are super common, here is, for example, an implementation for Vec. It would look something like this:

impl Default for Spec {
    fn default() -> Self {
        identity: String::From("https://lib.apollo.dev/core"),
        name: String::from("core"),
        version: Version(0, 1),
    }
}

TL;DR: for only internal library changes, Cow is brilliant, but if you're looking for ergonomics for you library users, go with Default.

P.S. So very sorry it took me so long to get back to you 🙈

Most of all, its worth noting that the code coverage results are not surfaced _anywhere_ right now as of this commit and there are no thresholds that hold us accountable for meeting or regressing on coverage requirements. However, the motivating factor in this removal is an error in the CI because of linker issues with profiling mechanisms that don't exist in WASM code. Put simply, this is not able to provide coverage for the code that we trust today. We absolutely do WANT code coverage, but the link to the [Stackoverflow post] in the comment within the code that introduced this current implementation (which this commit removes) states a very real obstacle and a very relevant issue. The introduction of this was arguably a bit premature given the relative infancy of the Rust code coverage tooling in the ecosystem and we never agreed on how we wanted to configure this. Hope is on the way (see attached), but for now let's just count on the fact that Rust's compiler is pretty good and Clippy (which we also have enabled on this repository) already points out dead-code and omissions to best practices. Alternatively, let's actually discuss how we want to configure this to really surface code coverage problems in the code we have in a meaningful way, not just running code coverage in CI for the sake of running code coverage in CI but not giving any meaningful output that indicates if it was coverage win or loss or anything.) Ref: https://blog.rust-lang.org/inside-rust/2020/11/12/source-based-code-coverage.html [Stackoverflow post]: https://stackoverflow.com/questions/61989746/how-do-i-use-zprofile-to-generate-code-coverage-for-a-nostd-kernel)

…tencies

queerviolet · 2021-03-02T14:39:07Z

PTAL, I think this is ready to go?

glasser · 2021-03-08T21:44:30Z

core-schema/src/version.rs

+
+/// Versions are a (major, minor) u64 pair.
+///
+/// Versions implement `PartialOrd` and `Ord`, which orders them by major and then


As with the TypeScript version, I'm not sure what the purpose of defining ordering (esp Ord) on versions is when satisfaction is the only semantically interesting property. Is this required in order to use Versions in some data structure?

... it looks like the answer is that we intend to use Versions as a key for a BTreeMap which more or less wants keys to be Ord. OK! So in this case we are defining PartialOrd/Ord less as an order that is deeply semantically meaningful to the application domain and more as something that lets us use them in certain data structures?

glasser · 2021-03-08T21:46:09Z

core-schema/src/version.rs

+/// *cannot* satisfy a request for `Version(1, 9)`. To check for version compatibility,
+/// use [the `satisfies` method](#satisfies).
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub struct Version(pub u64, pub u64);


I am not a Rust expert but it seems like it would be nice to use named fields here.

I am scared of a world where versions need 64 bits but I'm sure u64 is fine...

glasser · 2021-03-08T21:54:04Z

core-schema/src/version.rs

+    /// ```
+    pub fn parse(input: &str) -> Result<Version, VersionParseError> {
+        lazy_static! {
+            static ref VERSION_RE: Regex = Regex::new(r#"^v(\d+)\.(\d+)$"#).unwrap();


As in the TS version, this allows leading zeros which the spec bans. I do see that there's an explicit test for this below but I don't really think that the initial implementations of a spec's parser need to be lax in their parsing? The nice thing about banning leading zeroes is that it means each version has a unique serialization and you can compare them as strings directly if you'd like. Why not be strict?

You can replace each (\d+) with (0|[1-9]\d*) which is pretty legible I think...

glasser · 2021-03-08T21:57:36Z

core-schema/src/version.rs

+    }
+
+    #[test]
+    fn it_errors_on_invalid_specifiers() {


Suggested additional test cases: v1.2.3, v1, 1.2

glasser · 2021-03-08T22:07:05Z

core-schema/src/spec.rs

@@ -0,0 +1,100 @@
+//! Spec url handling
+//!
+//! `Spec`s are parsed from URL strings and extract the spec's:


Naming concern from apollographql/core-schema-js#2 (comment) applies here as well, though the fact that this is a struct rather than some kind of trait or whatever does make it a bit more clear to me.

glasser · 2021-03-08T23:21:23Z

core-schema/src/schema.rs

+    NoSchemas,
+
+    /// No reference to the core spec found
+    #[error("no reference to the core spec found")]


technically this is more like no reference to an understood version of the core spec is found. maybe the error can actually show the identity and version (or just the URL) that is understood?

glasser · 2021-03-08T23:22:01Z

core-schema/src/schema.rs

+    /// by the document. One will be selected, the others
+    /// will generate these errors.
+    #[error("{} extra using spec ignored", .0)]
+    ExtraUsing(Pos, Version),


This is unused (since you use find_map)

glasser · 2021-03-08T23:22:44Z

core-schema/src/schema.rs

+}
+
+impl SchemaError {
+    /// Collect `Request`s referenced by this error and join their position, urls,


Search whole project for "request" and rename to "feature"

glasser · 2021-03-08T23:23:55Z

core-schema/src/schema.rs

+                .iter()
+                .map(|req| {
+                    format!(
+                        "    {pos}: {url}/{ver}",


The as seems relevant to this error

glasser · 2021-03-08T23:27:52Z

core-schema/src/spec.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Spec {
+    pub identity: Cow<'static, str>,
+    pub name: Cow<'static, str>,


I worry it might be a little too easy to get confused between Spec.name and Feature.name and accidentally use the wrong one, writing some code that works as long as you don't use as. How important is it that there be a stand-alone name field on spec? It looks like the only time we ever read it is when we produce a Feature so I think we could just validate that the name is valid at Spec::parse time (and save name or its start index into a private field?) but make actually getting the name later be a method or something?

This commit is meant to be removed and replaced by a natural merge of `main` into this branch after #326 lands or rebasing this on `main` after #326 lands.

abernix · 2021-10-12T15:38:12Z

We can re-open this if we need to.

Ashi Krishnan added 5 commits January 13, 2021 13:45

Using crate

3b396bd

Support crate for using directive, move parser and query-planner back…

044a6bf

… to root.

Rename the various ParseErrors for clarity.

611457d

More test cases and overlapping prefix validation.

eadb045

Add tests for no schemas and too many schemas.

2c491d8

queerviolet requested review from lrlna and trevor-scheer January 14, 2021 16:02

queerviolet marked this pull request as draft January 14, 2021 16:09

queerviolet requested a review from abernix January 14, 2021 16:13

Ashi Krishnan added 12 commits January 14, 2021 13:15

Formatting for version, better docs and error handling in Schema

bb8a351

Version specifier tests.

7a9fa19

Docs and tests for version module

2add884

Documentation for request module.

b5cc83a

docs for constants and specs

f7c9d82

More doc comments for spec.

6595613

Implementations module.

f410fe3

Activating implementations.

b0940d1

Example of using Fns as implementations.

674dd0a

Remove unused import.

9103cff

Add a utility fn to remove dependency on drain_filter, which is still…

bfc8d9e

… nightly only.

cargo fmt

823f695

queerviolet marked this pull request as ready for review January 14, 2021 23:26

Ashi Krishnan added 2 commits January 15, 2021 11:01

cargo fmt

cf48368

cargo clippy

a53e1ba

Ashi Krishnan added 2 commits January 15, 2021 17:22

Change activations to return an iterator over all satisfying versions…

fce6354

… and create the Bounds trait to get bounds if desired.

Move snapshot inline.

c4093f0

lrlna reviewed Jan 19, 2021

View reviewed changes

abernix assigned queerviolet Feb 8, 2021

abernix force-pushed the qv/move-crates-to-top branch from d5f6ed9 to 373ab32 Compare February 10, 2021 13:40

Base automatically changed from qv/move-crates-to-top to main February 10, 2021 13:47

abernix changed the title ~~Rust implementation of the using spec~~ Rust implementation of the core spec Feb 11, 2021

abernix mentioned this pull request Feb 23, 2021

Introduce harmonizer crate and accompanying tool to compose schemas in Rust. #474

Merged

Merge remote-tracking branch 'origin/main' into qv/using

3fb2974

abernix requested a review from glasser February 24, 2021 13:19

Ashi Krishnan and others added 6 commits February 25, 2021 10:42

Rename using to core-schema and change @core(using:) to @core(feature:)

bc47795

Update failing tests.

46ce34a

chore(fmt): run cargo fmt to resolve graphql-parser format inconsis…

c6339a2

…tencies

Merge branch 'main' into qv/using

f0618bd

cargo fmt

79e7d94

abernix force-pushed the main branch from c6339a2 to 67e64c6 Compare February 26, 2021 13:37

abernix added 2 commits February 26, 2021 15:38

Merge branch 'main' into qv/using

4cabe75

Merge branch 'main' into qv/using

9dc6691

abernix requested a review from trevor-scheer March 2, 2021 15:15

glasser requested changes Mar 8, 2021

View reviewed changes

This was referenced Mar 11, 2021

Generate composite schema in spec'd form #305

Closed

[WIP] Reemergence of #305: Generation of composition in spec'd form #559

Closed

abernix marked this pull request as draft April 1, 2021 06:40

trevor-scheer removed their request for review October 12, 2021 15:33

abernix closed this Oct 12, 2021

abernix deleted the qv/using branch November 8, 2022 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust implementation of the core spec #326

Rust implementation of the core spec #326

queerviolet commented Jan 14, 2021 •

edited by abernix

Loading

queerviolet commented Jan 14, 2021 •

edited

Loading

queerviolet commented Jan 14, 2021

abernix commented Jan 15, 2021

lrlna left a comment

lrlna Jan 19, 2021

queerviolet Jan 19, 2021

lrlna Jan 19, 2021

queerviolet Jan 19, 2021

lrlna Jan 27, 2021

lrlna Jan 19, 2021

queerviolet Jan 19, 2021

lrlna Jan 27, 2021

lrlna Jan 19, 2021

lrlna Jan 19, 2021

lrlna Jan 19, 2021

lrlna Jan 19, 2021

lrlna Jan 19, 2021

lrlna Jan 19, 2021

queerviolet commented Jan 27, 2021

lrlna commented Feb 1, 2021

queerviolet commented Mar 2, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

glasser Mar 8, 2021

abernix commented Oct 12, 2021

	/// Return true iff this Version satisfies the `required` version
	/// Return true if this Version satisfies the `required` version


		use crate::spec::{Spec, SpecParseError};

		/// Requests contain a `spec`, the `prefix` requested for that spec (which

		@@ -0,0 +1,693 @@
		//! A GraphQL schema referencing one or more specifications

Rust implementation of the core spec #326

Rust implementation of the core spec #326

Conversation

queerviolet commented Jan 14, 2021 • edited by abernix Loading

queerviolet commented Jan 14, 2021 • edited Loading

queerviolet commented Jan 14, 2021

abernix commented Jan 15, 2021

lrlna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

queerviolet commented Jan 27, 2021

lrlna commented Feb 1, 2021

queerviolet commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix commented Oct 12, 2021

queerviolet commented Jan 14, 2021 •

edited by abernix

Loading

queerviolet commented Jan 14, 2021 •

edited

Loading