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.

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

feat: JsonLogic algebraic representation #297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShubhranshuSanjeev
Copy link
Collaborator

@ShubhranshuSanjeev ShubhranshuSanjeev commented Dec 4, 2024

Problem

No concrete type representation for JsonLogic in Rust, leading to clumsy parsing and custom logic to generate context conditions.

Solution

This PR introduces types such as:

  • Operands
  • Operator
  • Condition

which aims towards representing JsonLogic correctly in an algebraic fashion.
Along with the type, behaviors such as generating context JsonLogic, conversion from JsonLogic to these types are also added in this PR.

Alongside, generating forms from JsonLogic is handled in this PR, additionally logic to parse based on operators all defined at one place, for ease of use.

@ShubhranshuSanjeev ShubhranshuSanjeev requested a review from a team as a code owner December 4, 2024 07:43
@ShubhranshuSanjeev ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch 5 times, most recently from 9886f4d to 96705d5 Compare December 4, 2024 11:39
Comment on lines 12 to 13
Value(serde_json::Value),
Dimension(serde_json::Value),
Copy link
Collaborator

Choose a reason for hiding this comment

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

dimension will always be a string, can it be typed like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it will be a string, I am just keeping things simple, I am getting it as a json { "var": <dimension_name> }, so I am keeping the same structure, and not taking the dimension name out of the var.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better to take it out? Since "var" is useless in parsing later on

@@ -81,14 +81,18 @@ pub enum SchemaType {
impl SchemaType {
pub fn default_value(&self) -> Value {
match self {
SchemaType::Multiple(_) => json!(""),
SchemaType::Single(JsonSchemaType::String) => json!(""),
SchemaType::Multiple(_) => Value::String(String::default()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we write a comment on how to use SchemaType::Mulitple with some documentation at the type location?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So JsonSchema allows to give a list of type. type in schema can be either a string or a array of string example [boolean, integer]. The array corresponds to SchemaType::Multiple.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ShubhranshuSanjeev this definitely needs documentation. I assumed this represented a way to define multiple schemas for a type

Comment on lines 75 to 77
if !enum_variants.is_empty() {
return InputType::Select(enum_variants);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the else case for this be an error to the user
that they need to define at least one enum to allow editing to it

SchemaType::Single(JsonSchemaType::Integer) => InputType::Integer,
SchemaType::Single(JsonSchemaType::Boolean) => InputType::Toggle,
SchemaType::Single(JsonSchemaType::Null) => InputType::Disabled,
_ => InputType::Text,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we be explicit here, to make it better visible, so that if we are assigning a wrong input to a SchemaType it becomes more obvious

for example, here I suppose we should have provided Monaco input for Object and Array, right ?

Comment on lines 88 to 89
SchemaType::Single(JsonSchemaType::Number) => json!(0),
SchemaType::Single(JsonSchemaType::Integer) => json!(0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
SchemaType::Single(JsonSchemaType::Number) => json!(0),
SchemaType::Single(JsonSchemaType::Integer) => json!(0),
SchemaType::Single(JsonSchemaType::Number) => Value::Number(Number::from(0)),
SchemaType::Single(JsonSchemaType::Integer) => Value::Number(Number::from(0)),

@ShubhranshuSanjeev ShubhranshuSanjeev force-pushed the feat/json-logic-repr branch 2 times, most recently from 4411923 to 6fc31a5 Compare January 6, 2025 13:20
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.

3 participants