Skip to content

Commit

Permalink
Update the event-type CRUD to more strictly enforce Schema type (#668)
Browse files Browse the repository at this point in the history
The current event-type CRUD is too permissive with the `schema` field.
It accepts any json (such as `1`, "foobar", etc.), but it should only
accept json objects (such as `{"foo":"bar"}`).

This PR updates the event-type CRUD groups.

In addition, I wanted to support calling `json_wrapper!` outside of
types.rs. `types.rs` is already quite large, and I think it makes a bit
more sense for types (such as `eventtype::Schema`) to be colocated with
other types they interact with.

Finally, I updated the error logging to add trace information for 4xxs.
This wasn't strictly necessary, but it made diagnosing the source of
4xxs when testing locally much easier.
  • Loading branch information
svix-gabriel authored Oct 14, 2022
2 parents dd638dc + 4537b79 commit c49d17a
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 21 deletions.
21 changes: 13 additions & 8 deletions server/svix-server/src/core/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ macro_rules! enum_wrapper {
};
}

#[macro_export]
macro_rules! json_wrapper {
($name_id:ty) => {
impl From<$name_id> for sea_orm::Value {
Expand All @@ -98,37 +99,41 @@ macro_rules! json_wrapper {
}
}

impl TryGetable for $name_id {
fn try_get(res: &QueryResult, pre: &str, col: &str) -> Result<Self, TryGetError> {
impl sea_orm::TryGetable for $name_id {
fn try_get(
res: &QueryResult,
pre: &str,
col: &str,
) -> Result<Self, sea_orm::TryGetError> {
match Json::try_get(res, pre, col) {
Ok(v) => Ok(serde_json::from_value(v).expect("Error deserializing JSON")),
Err(e) => Err(e),
}
}
}

impl Nullable for $name_id {
impl sea_orm::sea_query::Nullable for $name_id {
fn null() -> Value {
Value::Json(None)
}
}

impl ValueType for $name_id {
fn try_from(v: Value) -> Result<Self, ValueTypeErr> {
impl sea_orm::sea_query::ValueType for $name_id {
fn try_from(v: Value) -> Result<Self, sea_orm::sea_query::ValueTypeErr> {
match v {
Value::Json(Some(x)) => {
Ok(serde_json::from_value(*x).expect("Error deserializing JSON"))
}
_ => Err(ValueTypeErr),
_ => Err(sea_orm::sea_query::ValueTypeErr),
}
}

fn type_name() -> String {
stringify!($name_id).to_owned()
}

fn column_type() -> ColumnType {
ColumnType::JsonBinary
fn column_type() -> sea_orm::sea_query::ColumnType {
sea_orm::sea_query::ColumnType::JsonBinary
}
}
};
Expand Down
14 changes: 12 additions & 2 deletions server/svix-server/src/db/models/eventtype.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
// SPDX-FileCopyrightText: © 2022 Svix Authors
// SPDX-License-Identifier: MIT

use crate::core::types::{BaseId, EventTypeId, EventTypeName, OrganizationId};
use std::collections::HashMap;

use crate::{
core::types::{BaseId, EventTypeId, EventTypeName, OrganizationId},
json_wrapper,
};
use chrono::Utc;
use sea_orm::entity::prelude::*;
use sea_orm::ActiveValue::Set;
use serde::{Deserialize, Serialize};

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
pub struct Schema(pub HashMap<String, Json>);
json_wrapper!(Schema);

#[derive(Clone, Debug, PartialEq, Eq, DeriveEntityModel)]
#[sea_orm(table_name = "eventtype")]
Expand All @@ -16,7 +26,7 @@ pub struct Model {
pub org_id: OrganizationId,
pub description: String,
pub deleted: bool,
pub schemas: Option<Json>,
pub schemas: Option<Schema>,
pub name: EventTypeName,
}

Expand Down
2 changes: 1 addition & 1 deletion server/svix-server/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl IntoResponse for Error {
fn into_response(self) -> Response {
match self.typ {
ErrorType::Http(s) => {
tracing::debug!("{:?}", &s);
tracing::debug!("{:?}, location: {:?}", &s, &self.trace);
s.into_response()
}
s => {
Expand Down
8 changes: 4 additions & 4 deletions server/svix-server/src/v1/endpoints/event_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub struct EventTypeIn {
pub description: String,
#[serde(default, rename = "archived")]
pub deleted: bool,
pub schemas: Option<serde_json::Value>,
pub schemas: Option<eventtype::Schema>,
}

// FIXME: This can and should be a derive macro
Expand Down Expand Up @@ -70,7 +70,7 @@ struct EventTypeUpdate {
description: String,
#[serde(default, rename = "archived")]
deleted: bool,
schemas: Option<serde_json::Value>,
schemas: Option<eventtype::Schema>,
}

// FIXME: This can and should be a derive macro
Expand Down Expand Up @@ -105,7 +105,7 @@ struct EventTypePatch {
deleted: UnrequiredField<bool>,

#[serde(default, skip_serializing_if = "UnrequiredNullableField::is_absent")]
schemas: UnrequiredNullableField<serde_json::Value>,
schemas: UnrequiredNullableField<eventtype::Schema>,
}

impl ModelIn for EventTypePatch {
Expand All @@ -131,7 +131,7 @@ pub struct EventTypeOut {
pub description: String,
#[serde(rename = "archived")]
pub deleted: bool,
pub schemas: Option<serde_json::Value>,
pub schemas: Option<eventtype::Schema>,

pub created_at: DateTime<Utc>,
pub updated_at: DateTime<Utc>,
Expand Down
13 changes: 9 additions & 4 deletions server/svix-server/tests/e2e_event_type.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
// SPDX-FileCopyrightText: © 2022 Svix Authors
// SPDX-License-Identifier: MIT

use std::collections::HashMap;

use reqwest::StatusCode;

use svix_server::v1::{
endpoints::event_type::{EventTypeIn, EventTypeOut},
utils::ListResponse,
use svix_server::{
db::models::eventtype::Schema,
v1::{
endpoints::event_type::{EventTypeIn, EventTypeOut},
utils::ListResponse,
},
};

mod utils;
Expand Down Expand Up @@ -79,7 +84,7 @@ async fn test_patch() {
.await
.unwrap();

assert_eq!(out.schemas, Some(serde_json::json!({})));
assert_eq!(out.schemas, Some(Schema(HashMap::new())));

// Assert the other fields remain unchanged
assert_eq!(out.deleted, et.deleted);
Expand Down
6 changes: 4 additions & 2 deletions server/svix-server/tests/utils/common_calls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,14 @@ pub async fn create_test_message(
.await
}

pub fn event_type_in<T: Serialize>(name: &str, payload: T) -> Result<EventTypeIn> {
pub fn event_type_in(name: &str, payload: serde_json::Value) -> Result<EventTypeIn> {
let schema = serde_json::from_value(payload).unwrap();

Ok(EventTypeIn {
name: EventTypeName(name.to_owned()),
description: "test-event-description".to_owned(),
deleted: false,
schemas: Some(serde_json::to_value(payload)?),
schemas: Some(schema),
})
}

Expand Down

0 comments on commit c49d17a

Please sign in to comment.