Restructure the diff resource so it is only possible to see diffs between revisions of the same article

src/resources/changes_resource.rs

@@ -12,7 +12,7 @@ use site::Layout;
 use state::State;
 use web::{Resource, ResponseFuture};
 
-use super::diff_resource::{self, ArticleRevisionReference};
+use super::diff_resource;
 use super::pagination::Pagination;
 use super::TemporaryRedirectResource;
 
@@ -337,10 +337,13 @@ impl Resource for ChangesResource {
                         _slug: x.slug,
                         title: x.title,
                         _latest: x.latest,
-                        diff_link: diff_resource::QueryParameters::new(
+                        diff_link: format!("_diff/{}?{}",
-                            ArticleRevisionReference::Some { article_id: x.article_id as u32, revision: x.revision as u32 - 1 },
+                            x.article_id,
-                            ArticleRevisionReference::Some { article_id: x.article_id as u32, revision: x.revision as u32 },
+                            diff_resource::QueryParameters::new(
-                        ).into_link()
+                                x.revision as u32 - 1,
+                                x.revision as u32,
+                            )
+                        ),
                     }
                 }).collect::<Vec<_>>();
 

src/resources/diff_resource.rs

@@ -1,108 +1,21 @@
-use std;
+use std::{self, fmt};
-use std::str::FromStr;
 
 use diff;
 use futures::{self, Future};
-use futures::future::{done, finished};
+use futures::future::done;
 use hyper;
 use hyper::header::ContentType;
 use hyper::server::*;
 use serde_urlencoded;
 
 use mimes::*;
-use models::ArticleRevision;
 use site::Layout;
 use state::State;
 use web::{Resource, ResponseFuture};
 
-const NONE: &str = "none";
-
 type BoxResource = Box<Resource + Sync + Send>;
 type Error = Box<std::error::Error + Send + Sync>;
 
-#[derive(Clone)]
-pub enum ArticleRevisionReference {
-    None,
-    Some {
-        article_id: u32,
-        revision: u32,
-    }
-}
-
-use std::num::ParseIntError;
-
-pub enum ArticleRevisionReferenceParseError {
-    SplitError,
-    ParseIntError(ParseIntError),
-}
-
-use std::fmt;
-impl fmt::Display for ArticleRevisionReferenceParseError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use self::ArticleRevisionReferenceParseError::*;
-        match self {
-            &SplitError => write!(f, "invalid format, must contain one @"),
-            &ParseIntError(ref r) => r.fmt(f)
-        }
-    }
-}
-
-impl From<ParseIntError> for ArticleRevisionReferenceParseError {
-    fn from(x: ParseIntError) -> ArticleRevisionReferenceParseError {
-        ArticleRevisionReferenceParseError::ParseIntError(x)
-    }
-}
-
-impl FromStr for ArticleRevisionReference {
-    type Err = ArticleRevisionReferenceParseError;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        if s == NONE {
-            return Ok(ArticleRevisionReference::None);
-        }
-
-        let items: Vec<&str> = s.split("@").collect();
-        if items.len() != 2 {
-            return Err(ArticleRevisionReferenceParseError::SplitError)
-        }
-
-        let article_id = items[0].parse::<u32>()?;
-        let revision = items[1].parse::<u32>()?;
-
-        Ok(ArticleRevisionReference::Some { article_id, revision })
-    }
-}
-
-impl fmt::Display for ArticleRevisionReference {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            &ArticleRevisionReference::None => write!(f, "{}", NONE),
-            &ArticleRevisionReference::Some { article_id, revision } => {
-                write!(f, "{}@{}", article_id, revision)
-            }
-        }
-    }
-}
-
-use serde::{de, Deserialize, Deserializer};
-impl<'de> Deserialize<'de> for ArticleRevisionReference {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-        where D: Deserializer<'de>
-    {
-        let s = String::deserialize(deserializer)?;
-        FromStr::from_str(&s).map_err(de::Error::custom)
-    }
-}
-
-use serde::{Serialize, Serializer};
-impl Serialize for ArticleRevisionReference {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-        where S: Serializer
-    {
-        serializer.serialize_str(&self.to_string())
-    }
-}
-
 #[derive(Clone)]
 pub struct DiffLookup {
     state: State,
@@ -110,17 +23,19 @@ pub struct DiffLookup {
 
 #[derive(Serialize, Deserialize)]
 pub struct QueryParameters {
-    from: ArticleRevisionReference,
+    from: u32,
-    to: ArticleRevisionReference,
+    to: u32,
 }
 
 impl QueryParameters {
-    pub fn new(from: ArticleRevisionReference, to: ArticleRevisionReference) -> QueryParameters {
+    pub fn new(from: u32, to: u32) -> QueryParameters {
         QueryParameters { from, to }
     }
+}
 
-    pub fn into_link(self) -> String {
+impl fmt::Display for QueryParameters {
-        format!("_diff?{}", serde_urlencoded::to_string(self).expect("Serializing to String cannot fail"))
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str(&serde_urlencoded::to_string(self).expect("Serializing to String cannot fail"))
     }
 }
 
@@ -129,26 +44,27 @@ impl DiffLookup {
         Self { state }
     }
 
-    pub fn lookup(&self, query: Option<&str>) -> Box<Future<Item=Option<BoxResource>, Error=::web::Error>> {
+    pub fn lookup(&self, article_id: u32, query: Option<&str>) -> Box<Future<Item=Option<BoxResource>, Error=::web::Error>> {
         let state = self.state.clone();
 
         Box::new(done((|| -> Result<Option<BoxResource>, ::web::Error> {
             let params: QueryParameters = serde_urlencoded::from_str(query.unwrap_or(""))?;
 
-            Ok(Some(Box::new(DiffResource::new(state, params.from, params.to))))
+            Ok(Some(Box::new(DiffResource::new(state, article_id, params.from, params.to))))
         }())))
     }
 }
 
 pub struct DiffResource {
     state: State,
-    from: ArticleRevisionReference,
+    article_id: u32,
-    to: ArticleRevisionReference,
+    from: u32,
+    to: u32,
 }
 
 impl DiffResource {
-    pub fn new(state: State, from: ArticleRevisionReference, to: ArticleRevisionReference) -> Self {
+    pub fn new(state: State, article_id: u32, from: u32, to: u32) -> Self {
-        Self { state, from, to }
+        Self { state, article_id, from, to }
     }
 
     fn query_args(&self) -> QueryParameters {
@@ -157,15 +73,6 @@ impl DiffResource {
             to: self.to.clone(),
         }
     }
-
-    fn get_article_revision(&self, r: &ArticleRevisionReference) -> Box<Future<Item = Option<ArticleRevision>, Error = Error>> {
-        match r {
-            &ArticleRevisionReference::None => Box::new(finished(None)),
-            &ArticleRevisionReference::Some { article_id, revision } => Box::new(
-                self.state.get_article_revision(article_id as i32, revision as i32)
-            ),
-        }
-    }
 }
 
 impl Resource for DiffResource {
@@ -196,8 +103,8 @@ impl Resource for DiffResource {
             added: Option<T>,
         }
 
-        let from = self.get_article_revision(&self.from);
+        let from = self.state.get_article_revision(self.article_id as i32, self.from as i32);
-        let to = self.get_article_revision(&self.to);
+        let to = self.state.get_article_revision(self.article_id as i32, self.to as i32);
 
         let head = self.head();
 
@@ -205,7 +112,7 @@ impl Resource for DiffResource {
             .and_then(move |(head, from, to)| {
                 Ok(head
                     .with_body(Layout {
-                        base: None, // Hmm, should perhaps accept `base` as argument
+                        base: Some("../"), // Hmm, should perhaps accept `base` as argument
                         title: "Difference",
                         body: &Template {
                             title: &diff::chars(

src/wiki_lookup.rs

@@ -137,6 +137,22 @@ impl WikiLookup {
         )
     }
 
+    fn diff_lookup_f(&self, path: &str, query: Option<&str>) -> <Self as Lookup>::Future {
+        let article_id: u32 = match (|| -> Result<_, <Self as Lookup>::Error> {
+            let (article_id, tail) = split_one(path)?;
+            if tail.is_some() {
+                return Err("Not found".into());
+            }
+
+            Ok(article_id.parse()?)
+        })() {
+            Ok(x) => x,
+            Err(_) => return Box::new(finished(None)),
+        };
+
+        Box::new(self.diff_lookup.lookup(article_id, query))
+    }
+
     fn reserved_lookup(&self, path: &str, query: Option<&str>) -> <Self as Lookup>::Future {
         let (head, tail) = match split_one(path) {
             Ok(x) => x,
@@ -154,8 +170,8 @@ impl WikiLookup {
                 self.by_id_lookup(tail, query),
             ("_changes", None) =>
                 Box::new(self.changes_lookup.lookup(query)),
-            ("_diff", None) =>
+            ("_diff", Some(tail)) =>
-                Box::new(self.diff_lookup.lookup(query)),
+                self.diff_lookup_f(tail, query),
             ("_new", None) =>
                 Box::new(finished(Some(Box::new(NewArticleResource::new(self.state.clone(), None)) as BoxResource))),
             ("_revisions", Some(tail)) =>