Skip to content

Commit

Permalink
Refactor: add top-level model module (#1317)
Browse files Browse the repository at this point in the history
This is the start of some backend refactoring. We noticed for a while
that many types don't cleanly fit into one submodule like `db`, `api`,
or something else, because they have DB impls, are loaded from the DB,
are exposed in the API and potentially other uses. Having a new top
level module house these make sense to me. I also don't mind having
smaller files in there, with fewer types per file. And I also don't mind
having many submodules inside `module`.

Note that this doesn't mean that we won't have two types for the same
"thing" at all anymore. Sometimes it makes sense to have a separate
type! For example, see some `search::*` types below which represent
exactly what's stored in the search index. We want to disconnect that
from the API representation somewhat, they don't necessarily need to
match.

This PR does not perform the full refactor, far from it! It just adds
the module and moves 3 interesting types already. To reduce merge
conflicts I expect us to move things around over the next couple months.

This is based on #1316, so that should be merged before.

---

I went through all modules (except `api`) and checked all types. Here
are more things I would move in the future:

- Pretty much everything in `db::types`
- `search::{TextSearchIndex, TextAssetType, TextMatch}` probably into
some `model::text`?
- `search::{Event, Playlist, Realm, Series, User}`: not sure about that.
These are mostly a representation of whats stored in the search index
and have a separate `SearchFoo` type in `api::model`. I think these can
stay. With one exception: `search::Playlist` currently does not have a
separate `SearchPlaylist` type. It should probably get one at some
point.
- Lots/most/all things in `api::model`? I haven't really looked at it
closely yet.

---

In terms of review: ehm... all the moving around and import changing is
annoying to review and I don't really see the point. Maybe roughly look
at the new structure? And maybe test a bit? Dunno, or nothing.
  • Loading branch information
owi92 authored Jan 20, 2025
2 parents d2ea178 + af600ea commit 5337026
Show file tree
Hide file tree
Showing 35 changed files with 264 additions and 215 deletions.
44 changes: 0 additions & 44 deletions backend/src/api/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use crate::{
},
prelude::*,
search::Playlist as SearchPlaylist,
db::types::ExtraMetadata,
};


Expand Down Expand Up @@ -100,46 +99,3 @@ impl Cursor {
Ok(Self(s.into()))
}
}

// TODO: This uses `graphql_scalar` instead of `derive(GraphQLScalar)` because
// the type `ExtraMetadata` is defined in the `db` module and adding GraphQL
// code there seems wrong. However, I feel like we should move some types
// around anyway since we encountered problems like this before.
#[juniper::graphql_scalar(
name = "ExtraMetadata",
description = "Arbitrary metadata for events/series. Serialized as JSON object.",
with = Self,
parse_token(String),
)]
#[allow(dead_code)]
pub type ApiExtraMetadata = ExtraMetadata;

impl ExtraMetadata {
fn to_output<S: ScalarValue>(&self) -> juniper::Value<S> {
use juniper::Value;

std::iter::once(("dcterms", &self.dcterms))
.chain(self.extended.iter().map(|(k, v)| (&**k, v)))
.map(|(k, v)| {
let value = v.iter()
.map(|(k, v)| {
let elements = v.iter()
.map(|s| Value::Scalar(S::from(s.clone())))
.collect();
(k, Value::List(elements))
})
.collect::<juniper::Object<S>>();

(k, Value::Object(value))
})
.collect::<juniper::Object<S>>()
.pipe(Value::Object)
}

fn from_input<S: ScalarValue>(input: &InputValue<S>) -> Result<Self, String> {
// I did not want to waste time implementing this now, given that we
// likely never use it.
let _ = input;
todo!("ExtraMetadata cannot be used as input value yet")
}
}
51 changes: 3 additions & 48 deletions backend/src/api/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize};
use static_assertions::const_assert;
use std::fmt;

use crate::{db::types::Key, util::{base64_decode, BASE64_DIGITS}};
use crate::model::Key;


/// An opaque, globally-unique identifier for all "nodes" that the GraphQL API
Expand Down Expand Up @@ -138,30 +138,6 @@ impl Id {
}
}

impl Key {
pub(crate) fn from_base64(s: &str) -> Option<Self> {
if s.len() != 11 {
return None;
}

decode_base64(s.as_bytes())
}

pub(crate) fn to_base64<'a>(&self, out: &'a mut [u8; 11]) -> &'a str {
// Base64 encoding. After this loop, `n` is always 0, because `u64::MAX`
// divided by 64 eleven times is 0.
let mut n = self.0;
for i in (0..out.len()).rev() {
out[i] = BASE64_DIGITS[(n % 64) as usize];
n /= 64;
}
debug_assert!(n == 0);

std::str::from_utf8(out)
.expect("bug: base64 did produce non-ASCII character")
}
}

impl std::str::FromStr for Id {
type Err = &'static str;

Expand Down Expand Up @@ -191,31 +167,10 @@ impl fmt::Display for Id {
}
}


fn decode_base64(src: &[u8]) -> Option<Key> {
let src: [u8; 11] = src.try_into().ok()?;

// Make sure the string doesn't decode to a number > `u64::MAX`. Luckily,
// checking that is easy. `u64::MAX` encodes to `P__________`, so the next
// higher number would carry through and make the highest digit a `Q`. So we
// just make sure the first digit is between 'A' and 'P'.
if src[0] > b'P' || src[0] < b'A' {
return None;
}

src.iter()
.rev()
.enumerate()
.map(|(i, &d)| base64_decode(d).map(|n| n as u64 * 64u64.pow(i as u32)))
.sum::<Option<u64>>()
.map(Key)
}


#[cfg(test)]
mod tests {
use std::str::FromStr;
use super::{Id, Key, BASE64_DIGITS};
use super::{Id, Key};

#[test]
fn simple() {
Expand Down Expand Up @@ -265,7 +220,7 @@ mod tests {
let id = Id { kind: Id::REALM_KIND, key: Key((n as u64) << shift) };
let s = id.to_string();
assert_eq!(s[..2].as_bytes(), Id::REALM_KIND);
assert!(s[2..].bytes().all(|d| BASE64_DIGITS.contains(&d)));
assert!(s[2..].bytes().all(|d| crate::util::BASE64_DIGITS.contains(&d)));
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion backend/src/api/model/acl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use juniper::{GraphQLInputObject, GraphQLObject};
use postgres_types::BorrowToSql;
use serde::Serialize;

use crate::{api::{err::ApiResult, Context}, config::TranslatedString, db::util::select};
use crate::{api::{err::ApiResult, Context}, model::TranslatedString, db::util::select};



Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/block/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use crate::{
Context,
Id,
},
db::{types::Key, util::impl_from_db},
model::Key,
db::util::impl_from_db,
prelude::*,
};

Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/block/mutations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ use juniper::{GraphQLInputObject, GraphQLObject};

use crate::{
api::{Context, Id, err::{ApiResult, invalid_input}, model::realm::Realm},
db::{types::Key, util::select},
db::util::select,
model::Key,
prelude::*,
};
use super::{BlockValue, VideoListOrder, VideoListLayout};
Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ use crate::{
},
},
db::{
types::{EventCaption, EventSegment, EventState, EventTrack, ExtraMetadata, Key, Credentials},
types::{EventCaption, EventSegment, EventState, EventTrack, Credentials},
util::{impl_from_db, select},
},
model::{Key, ExtraMetadata},
prelude::*,
};

Expand Down
2 changes: 1 addition & 1 deletion backend/src/api/model/known_roles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use serde::Deserialize;

use crate::{
api::{err::ApiResult, Context},
config::TranslatedString,
model::TranslatedString,
db::util::{impl_from_db, select},
prelude::*,
};
Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/playlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use crate::{
api::{
common::NotAllowed, err::ApiResult, Context, Id, Node, NodeValue
},
db::{types::Key, util::{impl_from_db, select}},
db::util::{impl_from_db, select},
model::Key,
prelude::*,
};

Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/realm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use crate::{
NodeValue,
},
auth::AuthContext,
db::{types::Key, util::{impl_from_db, select}},
db::util::{impl_from_db, select},
model::Key,
prelude::*,
};
use super::block::BlockValue;
Expand Down
2 changes: 1 addition & 1 deletion backend/src/api/model/realm/mutations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
model::block::RemovedBlock,
},
auth::AuthContext,
db::types::Key,
model::Key,
prelude::*,
};
use super::{Realm, RealmOrder};
Expand Down
3 changes: 2 additions & 1 deletion backend/src/api/model/series.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use crate::{
err::{invalid_input, ApiResult},
model::{event::AuthorizedEvent, realm::Realm},
},
db::{types::{ExtraMetadata, Key, SeriesState as State}, util::impl_from_db},
db::{types::{SeriesState as State}, util::impl_from_db},
model::{Key, ExtraMetadata},
prelude::*,
};

Expand Down
2 changes: 1 addition & 1 deletion backend/src/auth/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use hyper::{http::HeaderName, Uri};
use secrecy::SecretString;
use serde::{Deserialize, Deserializer, de::Error};

use crate::{config::{parse_normal_http_uri, TranslatedString}, prelude::*};
use crate::{config::parse_normal_http_uri, model::TranslatedString, prelude::*};

use super::JwtConfig;

Expand Down
3 changes: 2 additions & 1 deletion backend/src/cmd/known_groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use serde_json::json;

use crate::{
api::model::known_roles::KnownGroup,
config::{Config, TranslatedString},
config::Config,
model::TranslatedString,
db,
prelude::*,
};
Expand Down
3 changes: 2 additions & 1 deletion backend/src/config/general.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::collections::HashMap;

use super::{HttpHost, TranslatedString};
use crate::model::TranslatedString;
use super::HttpHost;


#[derive(Debug, confique::Config)]
Expand Down
2 changes: 0 additions & 2 deletions backend/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ use crate::prelude::*;
mod color;
mod general;
mod theme;
mod translated_string;
mod matomo;
mod opencast;
mod player;
mod upload;

pub(crate) use self::{
translated_string::TranslatedString,
theme::{ThemeConfig, LogoDef},
matomo::MatomoConfig,
opencast::OpencastConfig,
Expand Down
3 changes: 2 additions & 1 deletion backend/src/config/theme.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::{collections::HashMap, fmt, path::PathBuf};
use serde::{Deserialize, Serialize};

use super::{color::ColorConfig, translated_string::LangKey};
use crate::model::LangKey;
use super::{color::ColorConfig};


#[derive(Debug, confique::Config)]
Expand Down
2 changes: 1 addition & 1 deletion backend/src/db/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{prelude::*, db::types::Key};
use crate::{prelude::*, model::Key};
use super::DbConfig;
use self::util::TestDb;

Expand Down
2 changes: 1 addition & 1 deletion backend/src/db/tests/search_queue.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{prelude::*, db::types::Key, search::IndexItemKind};
use crate::{prelude::*, model::Key, search::IndexItemKind};
use super::util::TestDb;


Expand Down
2 changes: 1 addition & 1 deletion backend/src/db/tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{ops::Deref, collections::HashSet};
use secrecy::ExposeSecret;
use tokio_postgres::{Client, NoTls};

use crate::{prelude::*, db::types::Key, search::IndexItemKind};
use crate::{prelude::*, model::Key, search::IndexItemKind};
use super::DbConfig;


Expand Down
Loading

0 comments on commit 5337026

Please sign in to comment.