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.

Sign up for GitHub

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

Expose hint levels #200

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Expose hint levels #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea35322
Expose hint levels
pcmanus Oct 7, 2022
fd9c0fb
Merge branch 'main' into hint-levels
EverlastingBugstopper Oct 25, 2022
6e7fe0d
Merge branch 'main' into hint-levels
EverlastingBugstopper Nov 7, 2022
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
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

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

4 changes: 3 additions & 1 deletion apollo-federation-types/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ config = ["camino", "log", "thiserror", "serde_yaml", "url", "serde_with"]
[dependencies]
# config and build dependencies
serde = { version = "1", features = ["derive"] }
regex = "1"
lazy_static = "1.4.0"

# config-only dependencies
camino = { version = "1", features = [ "serde1" ], optional = true }
Expand All @@ -34,4 +36,4 @@ serde_json = { version = "1", optional = true }

[dev-dependencies]
assert_fs = "1"
serde_json = "1"
serde_json = "1"
91 changes: 86 additions & 5 deletions apollo-federation-types/src/build/hint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,76 @@
use serde::{Deserialize, Serialize};
use regex::Regex;
use lazy_static::lazy_static;

#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
pub struct BuildHintLevel {
/// Value of the hint level. Higher values correspond to more "important" hints.
pub value: u16,

/// Readable name of the hint level
pub name: String,
}

impl BuildHintLevel {
pub fn warn() -> Self { Self { value: 60, name: String::from("WARN") } }
pub fn info() -> Self { Self { value: 40, name: String::from("INFO") } }
pub fn debug() -> Self { Self { value: 20, name: String::from("DEBUG") } }
}


/// BuildHint contains helpful information that pertains to a build
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
pub struct BuildHint {
/// The message of the hint
/// This will usually be formatted as "[<hint code>] <message details>" and the
/// `BuildHint::extract_code_and_message` method can be used to extract the components of this
/// message.
pub message: String,

/// The level of the hint
// We should always get a level out of recent harmonizer, but older one will not have it and we
// default to "INFO".
#[serde(default="BuildHintLevel::info")]
pub level: BuildHintLevel,

/// Other untyped JSON included in the build hint.
#[serde(flatten)]
pub other: crate::UncaughtJson,
}

impl BuildHint {
pub fn new(message: String) -> Self {
pub fn new(message: String, level: BuildHintLevel) -> Self {
Self {
message,
level,
other: crate::UncaughtJson::new(),
}
}

pub fn debug(message: String) -> Self {
Self::new(message, BuildHintLevel::debug())
}

pub fn info(message: String) -> Self {
Self::new(message, BuildHintLevel::info())
}

pub fn warn(message: String) -> Self {
Self::new(message, BuildHintLevel::warn())
}

/// Extracts the underlying code and "raw" message of the hint.
pub fn extract_code_and_message(&self) -> (String, String) {
lazy_static! {
static ref RE: Regex = Regex::new(r"^\[(\w+)\] (.+)").unwrap();
}
let maybe_captures = RE.captures(&self.message);
if let Some(captures) = maybe_captures {
(captures.get(1).unwrap().as_str().to_string(), captures.get(2).unwrap().as_str().to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a more structured way that we can get this data from the underlying JS library? I don't love the need to use a regex here for this - if we could instead put that info in a JSON object or something instead of parsing out the string I think this will be more robust

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a huge fan either, but I was worried about either making it weird to use, or have backward compatibility problems.

Let me however remark that this method is not about the hint "level". A hint has fundamentally 3 components at the moment:

  1. the message, which describe the particular hint.
  2. the code, which categorize hints (same as we have for errors).
  3. the level, which is WARN, INFO or DEBUG for now.

The level, that is introduced by this patch, is shipped as a separate field in the underlying JSON object, and thus deserialized separately, so no worry on this one.

But what this method is trying to do is give access to the message and code separately. Because the issue is that unfortunately, instead of having the message and code as separate fields in the underlying JSON, do_compose.js has been using the Hint.toString() method to generate a single pre-formatted string of the form "[<code>] <message>" and calling this "message".

So first, I acknowledge that this isn't strictly hint level related and so one could make the argument this shouldn't be in this patch. But I think the ultimate goal should be that in the rover UX, errors and hints can be displayed in a consistent way, where errors can just be considered as the highest "hint level" (in fact, we do hope to "merge" hints and errors in the code at some point in the future to clean things up). But doing that cleanly implies rover can access the hint code and message separately, the same way it does for errors, so it can maybe colorize the code and/or format it however it pleases. This could also allow flexibility like grouping messages by code for both hints and errors, or filtering certain code, etc.

Long story short, the current BuildHint.message field of hint.rs is currently unfortunately not really just the hint message, it is a pre-formatted string with both the code and (raw) message and this method is meant to provide a way out of this.

In theory, a better alternative would be to change BuildHint.message to only be the actual hint message and to add a BuildHint.code for the code, and to maybe also ensure those are separate in the underlying JSON object so it's easier to do, but I was kind of worried about breaking things by doing those things. So the idea of this method was to keep BuiltHint.message unchanged so we don't break existing usages, but to allow future usages to be able to access the code and message components individually.

Anyway, hope this clarify. If you're not worry about the concerns around breaking existing code, I'm happy to change all this, but I think it would require some care around versioning this properly, because if we change this, currently rover version would need to not use federation-rs versions with such changes (or rather, if they do, the hint will suddenly not be displayed with their code since it wouldn't be part of BuildHint.message anymore).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have to worry about breaking existing code! We should definitely change the underlying library code away from the .toString here - the only thing that matters is that we don't remove fields from the JSON that's actually emitted by the supergraph binary. Everything else is an internal contract - we only need to hold up the end to end guarantee.

Copy link
Author

Choose a reason for hiding this comment

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

the only thing that matters is that we don't remove fields from the JSON that's actually emitted by the supergraph binary

But currently, the JSON of the supergraph binary has the message field for hints that is the pre-formatted string containing both the "raw" message and the code. And existing rover versions kind of assume this is what this field contains, but we ultimately want rover to be able to access separately and not as a pre-formatted string. So if we do the "cleanest" change here without backward compatibility concerns, we would be changing that message field to now be only the "raw" message, without code, and the code would be a new field. Technically this is not a field removal, but it's changing what the field holds, and I assume that would be an issue too?

The alternative could be keep the message field containing the pre-formatted string (we can still stop having the pre-formatted string in the JSON from javascript, but the supergraph binary itself would be the one formatting the string for its own JSON output), but also add 2 new fields, code and raw_message, with the message field essentially becoming deprecated but kept for backward compatibility.

Does that make sense, or am I overcomplicating this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is not a field removal, but it's changing what the field holds, and I assume that would be an issue too?

Yeah - I think you're right here, we want older versions to still be able to work.

The alternative could be keep the message field containing the pre-formatted string (we can still stop having the pre-formatted string in the JSON from javascript, but the supergraph binary itself would be the one formatting the string for its own JSON output), but also add 2 new fields, code and raw_message, with the message field essentially becoming deprecated but kept for backward compatibility.

Old versions of Rover will discard any new unused JSON fields (we should test this to verify of course), but this approach sounds perfectly reasonable to me!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I'll try to update the patch accordingly ... when I find a bit of time for it.

} else {
(String::from("UNKNOWN"), self.message.clone())
}
}
}

#[cfg(test)]
Expand All @@ -29,16 +82,24 @@ mod tests {
#[test]
fn it_can_serialize() {
let msg = "hint".to_string();
let expected_json = json!({ "message": &msg });
let actual_json = serde_json::to_value(&BuildHint::new(msg)).unwrap();
let expected_json = json!({"level": { "value": 40, "name": "INFO"}, "message": &msg });
let actual_json = serde_json::to_value(&BuildHint::info(msg)).unwrap();
assert_eq!(expected_json, actual_json)
}

#[test]
fn it_can_deserialize() {
let msg = "hint".to_string();
let actual_struct = serde_json::from_str(&json!({"level": { "value": 20, "name": "DEBUG"}, "message": &msg }).to_string()).unwrap();
let expected_struct = BuildHint::debug(msg);
assert_eq!(expected_struct, actual_struct);
}

#[test]
fn it_can_deserialize_without_levels() {
let msg = "hint".to_string();
let actual_struct = serde_json::from_str(&json!({ "message": &msg }).to_string()).unwrap();
let expected_struct = BuildHint::new(msg);
let expected_struct = BuildHint::info(msg);
assert_eq!(expected_struct, actual_struct);
}

Expand All @@ -51,10 +112,30 @@ mod tests {
&json!({ "message": &msg, &unexpected_key: &unexpected_value }).to_string(),
)
.unwrap();
let mut expected_struct = BuildHint::new(msg);
let mut expected_struct = BuildHint::info(msg);
expected_struct
.other
.insert(unexpected_key, Value::String(unexpected_value));
assert_eq!(expected_struct, actual_struct);
}

#[test]
fn it_extracts_code_and_message() {
let hint = BuildHint::info("[MY_CODE] Some message".to_string());
let (actual_code, actual_message) = hint.extract_code_and_message();
let expected_code = "MY_CODE".to_string();
let expected_message = "Some message".to_string();
assert_eq!(expected_code, actual_code);
assert_eq!(expected_message, actual_message);
}

#[test]
fn it_handle_extracting_code_and_message_with_unknown_code() {
let hint = BuildHint::info("Some message without code".to_string());
let (actual_code, actual_message) = hint.extract_code_and_message();
let expected_code = "UNKNOWN".to_string();
let expected_message = "Some message without code".to_string();
assert_eq!(expected_code, actual_code);
assert_eq!(expected_message, actual_message);
}
}
6 changes: 3 additions & 3 deletions apollo-federation-types/src/build/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ mod tests {
let sdl = "my-sdl".to_string();
let hint_one = "hint-one".to_string();
let hint_two = "hint-two".to_string();
let expected_json = json!({"supergraphSdl": &sdl, "hints": [{"message": &hint_one}, {"message": &hint_two}]});
let expected_json = json!({"supergraphSdl": &sdl, "hints": [{"level": { "value": 40, "name": "INFO"}, "message": &hint_one}, {"level": { "value": 60, "name": "WARN"}, "message": &hint_two}]});
let actual_json = serde_json::to_value(&BuildOutput::new_with_hints(
sdl.to_string(),
vec![BuildHint::new(hint_one), BuildHint::new(hint_two)],
vec![BuildHint::info(hint_one), BuildHint::warn(hint_two)],
))
.unwrap();
assert_eq!(expected_json, actual_json)
Expand All @@ -81,7 +81,7 @@ mod tests {
.unwrap();
let expected_struct = BuildOutput::new_with_hints(
sdl,
vec![BuildHint::new(hint_one), BuildHint::new(hint_two)],
vec![BuildHint::info(hint_one), BuildHint::info(hint_two)],
);

assert_eq!(expected_struct, actual_struct)
Expand Down
2 changes: 2 additions & 0 deletions federation-2/Cargo.lock
View file
Open in desktop

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

5 changes: 4 additions & 1 deletion federation-2/harmonizer/deno/do_compose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ try {
let hints = [];
if (composed.hints) {
composed.hints.map((composed_hint) => {
hints.push({ message: composed_hint.toString() });
hints.push({
message: composed_hint.toString(),
level: composed_hint.definition.level,
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we are putting that info right into the JSON object here in the JS code - can we just do something like

let data = serde_json::from_string(data_from_js);
let message = data["message"];
let level = data["level"];

?

});
});
}
done(
Expand Down