Skip to content
6 changes: 6 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ pub enum RenderError {
#[label("{object}")]
object_at: Option<SourceSpan>,
},
#[error("'{type_name}' object is not subscriptable")]
NotSubscriptable {
type_name: String,
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we can use &'static str here to avoid an allocation:

Suggested change
type_name: String,
type_name: &'static str,

Not a problem to leave it as is if this doesn't compile.

Copy link
Owner

Choose a reason for hiding this comment

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

I checked, and we can do this alongside the PyErr::annotate change.

#[label("filter applied here")]
at: SourceSpan,
},
}

pub trait AnnotatePyErr {
Expand Down
13 changes: 13 additions & 0 deletions src/filters.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::sync::Arc;

use miette::SourceSpan;
use pyo3::prelude::*;

use crate::types::Argument;
Expand All @@ -13,6 +14,7 @@ pub enum FilterType {
Default(DefaultFilter),
Escape(EscapeFilter),
External(ExternalFilter),
First(FirstFilter),
Lower(LowerFilter),
Safe(SafeFilter),
Slugify(SlugifyFilter),
Expand Down Expand Up @@ -67,6 +69,17 @@ pub struct ExternalFilter {
pub argument: Option<Argument>,
}

#[derive(Clone, Debug, PartialEq)]
pub struct FirstFilter {
pub at: SourceSpan,
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't really matter, but I'd keep at as a (usize, usize) until using it in RenderError:

Suggested change
pub at: SourceSpan,
pub at: (usize, usize),

The reason is basically just to avoid using the third party type (miette::SourceSpan) until it's necessary.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I think this change will be necessary to use PyErr::annotate.

}

impl FirstFilter {
pub fn new(at: SourceSpan) -> Self {
Self { at }
}
}

impl ExternalFilter {
pub fn new(filter: Py<PyAny>, argument: Option<Argument>) -> Self {
Self {
Expand Down
5 changes: 5 additions & 0 deletions src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::filters::DefaultFilter;
use crate::filters::EscapeFilter;
use crate::filters::ExternalFilter;
use crate::filters::FilterType;
use crate::filters::FirstFilter;
use crate::filters::LowerFilter;
use crate::filters::SafeFilter;
use crate::filters::SlugifyFilter;
Expand Down Expand Up @@ -125,6 +126,10 @@ impl Filter {
Some(right) => return Err(unexpected_argument("escape", right)),
None => FilterType::Escape(EscapeFilter),
},
"first" => match right {
Some(right) => return Err(unexpected_argument("first", right)),
None => FilterType::First(FirstFilter::new(at.into())),
},
"lower" => match right {
Some(right) => return Err(unexpected_argument("lower", right)),
None => FilterType::Lower(LowerFilter),
Expand Down
167 changes: 166 additions & 1 deletion src/render/filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ use std::sync::LazyLock;
use html_escape::encode_quoted_attribute_to_string;
use num_bigint::{BigInt, ToBigInt};
use num_traits::ToPrimitive;
use pyo3::exceptions::PyIndexError;
use pyo3::prelude::*;
use pyo3::sync::GILOnceCell;
use pyo3::types::PyType;

use crate::error::RenderError;
use crate::filters::{
AddFilter, AddSlashesFilter, CapfirstFilter, CenterFilter, DefaultFilter, EscapeFilter,
ExternalFilter, FilterType, LowerFilter, SafeFilter, SlugifyFilter, UpperFilter,
ExternalFilter, FilterType, FirstFilter, LowerFilter, SafeFilter, SlugifyFilter, UpperFilter,
};
use crate::parse::Filter;
use crate::render::types::{AsBorrowedContent, Content, ContentString, Context, IntoOwnedContent};
Expand Down Expand Up @@ -47,6 +48,7 @@ impl Resolve for Filter {
FilterType::Default(filter) => filter.resolve(left, py, template, context),
FilterType::Escape(filter) => filter.resolve(left, py, template, context),
FilterType::External(filter) => filter.resolve(left, py, template, context),
FilterType::First(filter) => filter.resolve(left, py, template, context),
FilterType::Lower(filter) => filter.resolve(left, py, template, context),
FilterType::Safe(filter) => filter.resolve(left, py, template, context),
FilterType::Slugify(filter) => filter.resolve(left, py, template, context),
Expand Down Expand Up @@ -308,6 +310,89 @@ impl ResolveFilter for ExternalFilter {
}
}

impl ResolveFilter for FirstFilter {
fn resolve<'t, 'py>(
&self,
variable: Option<Content<'t, 'py>>,
py: Python<'py>,
_template: TemplateString<'t>,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
_template: TemplateString<'t>,
template: TemplateString<'t>,

_context: &mut Context,
) -> ResolveResult<'t, 'py> {
let content = match variable {
Some(content) => content,
None => return Ok(Some("".as_content())),
};

match content {
Content::Py(obj) => {
// Try to get the first item using Python's indexing
// Django only catches IndexError, not TypeError
match obj.get_item(0) {
Ok(first) => Ok(Some(Content::Py(first))),
Err(e) if e.is_instance_of::<PyIndexError>(py) => Ok(Some("".as_content())),
Err(e) if e.is_instance_of::<pyo3::exceptions::PyTypeError>(py) => {
// Check if this is the standard "'type' object is not subscriptable" error
let error_msg = e.to_string();
if error_msg.contains("is not subscriptable") {
// Standard not subscriptable error - provide better formatting
let type_name = obj
.get_type()
.name()
.map(|n| n.to_string())
.unwrap_or_else(|_| "unknown".to_string());
Err(RenderError::NotSubscriptable {
type_name,
at: self.at,
}
.into())
} else {
// Custom TypeError - preserve the original message
Err(e.into())
}
}
Err(e) => Err(e.into()),
Comment on lines +333 to +353
Copy link
Owner

Choose a reason for hiding this comment

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

Since my last review, I've added a PyErr::annotate method which I think we can use here:

Suggested change
Err(e) if e.is_instance_of::<pyo3::exceptions::PyTypeError>(py) => {
// Check if this is the standard "'type' object is not subscriptable" error
let error_msg = e.to_string();
if error_msg.contains("is not subscriptable") {
// Standard not subscriptable error - provide better formatting
let type_name = obj
.get_type()
.name()
.map(|n| n.to_string())
.unwrap_or_else(|_| "unknown".to_string());
Err(RenderError::NotSubscriptable {
type_name,
at: self.at,
}
.into())
} else {
// Custom TypeError - preserve the original message
Err(e.into())
}
}
Err(e) => Err(e.into()),
Err(error) => {
let error = error.annotate(py, self.at, "here", template);
Err(error.into())
}

}
}
Content::String(s) => {
// For strings, get the first character
// Skip the empty string always present at the start after splitting
let mut chars = s.as_raw().split("").skip(1);
match chars.next() {
None => Ok(Some("".as_content())),
Some(c) => Ok(Some(c.to_string().into_content())),
}
}
Content::Int(_) => {
// Numbers are not sequences, should raise TypeError
// Match Django's behavior exactly
Err(RenderError::NotSubscriptable {
type_name: "int".to_string(),
Copy link
Owner

Choose a reason for hiding this comment

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

If type_name can be &'static str, we can avoid the to_string() call:

Suggested change
type_name: "int".to_string(),
type_name: "int",

at: self.at,
}
.into())
}
Content::Float(_) => {
// Floats are not sequences, should raise TypeError
// Match Django's behavior exactly
Err(RenderError::NotSubscriptable {
type_name: "float".to_string(),
at: self.at,
}
.into())
}
Content::Bool(_) => {
// Booleans are not sequences, should raise TypeError
// Match Django's behavior exactly
Err(RenderError::NotSubscriptable {
type_name: "bool".to_string(),
at: self.at,
}
.into())
}
}
Comment on lines +356 to +392
Copy link
Owner

Choose a reason for hiding this comment

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

We need tests for these.

}
}

impl ResolveFilter for LowerFilter {
fn resolve<'t, 'py>(
&self,
Expand Down Expand Up @@ -981,4 +1066,84 @@ mod tests {
assert_eq!(rendered, "");
})
}

#[test]
fn test_render_filter_first_list() {
pyo3::prepare_freethreaded_python();

Python::with_gil(|py| {
let engine = EngineData::empty();
let template_string = "{{ items|first }}".to_string();
let context = PyDict::new(py);
let items = vec!["a", "b", "c"];
context.set_item("items", items).unwrap();
let template = Template::new_from_string(py, template_string, &engine).unwrap();
let result = template.render(py, Some(context), None).unwrap();

assert_eq!(result, "a");
})
}

#[test]
fn test_render_filter_first_empty_list() {
pyo3::prepare_freethreaded_python();

Python::with_gil(|py| {
let engine = EngineData::empty();
let template_string = "{{ items|first }}".to_string();
let context = PyDict::new(py);
let items: Vec<&str> = vec![];
context.set_item("items", items).unwrap();
let template = Template::new_from_string(py, template_string, &engine).unwrap();
let result = template.render(py, Some(context), None).unwrap();

assert_eq!(result, "");
})
}

#[test]
fn test_render_filter_first_string() {
pyo3::prepare_freethreaded_python();

Python::with_gil(|py| {
let engine = EngineData::empty();
let template_string = "{{ text|first }}".to_string();
let context = PyDict::new(py);
context.set_item("text", "hello").unwrap();
let template = Template::new_from_string(py, template_string, &engine).unwrap();
let result = template.render(py, Some(context), None).unwrap();

assert_eq!(result, "h");
})
}

#[test]
fn test_render_filter_first_empty_string() {
pyo3::prepare_freethreaded_python();

Python::with_gil(|py| {
let engine = EngineData::empty();
let template_string = "{{ text|first }}".to_string();
let context = PyDict::new(py);
context.set_item("text", "").unwrap();
let template = Template::new_from_string(py, template_string, &engine).unwrap();
let result = template.render(py, Some(context), None).unwrap();

assert_eq!(result, "");
})
}

#[test]
fn test_render_filter_first_no_argument() {
pyo3::prepare_freethreaded_python();

Python::with_gil(|py| {
let engine = EngineData::empty();
let template_string = "{{ items|first:'arg' }}".to_string();
let error = Template::new_from_string(py, template_string, &engine).unwrap_err();

let error_string = format!("{error}");
assert!(error_string.contains("first filter does not take an argument"));
})
}
}
18 changes: 17 additions & 1 deletion src/template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub mod django_rusty_templates {
use std::path::PathBuf;

use encoding_rs::Encoding;
use pyo3::exceptions::{PyAttributeError, PyImportError, PyOverflowError, PyValueError};
use pyo3::exceptions::{PyAttributeError, PyImportError, PyOverflowError, PyTypeError, PyValueError};
use pyo3::import_exception_bound;
use pyo3::intern;
use pyo3::prelude::*;
Expand Down Expand Up @@ -77,6 +77,16 @@ pub mod django_rusty_templates {
}
}

impl WithSourceCode for PyTypeError {
fn with_source_code(
err: miette::Report,
source: impl miette::SourceCode + 'static,
) -> PyErr {
let miette_err = err.with_source_code(source);
Self::new_err(format!("{miette_err:?}"))
}
}

pub struct EngineData {
autoescape: bool,
libraries: HashMap<String, Py<PyAny>>,
Expand Down Expand Up @@ -315,6 +325,12 @@ pub mod django_rusty_templates {
Err(err) => {
let err = err.try_into_render_error()?;
match err {
RenderError::NotSubscriptable { .. } => {
return Err(PyTypeError::with_source_code(
err.into(),
self.template.clone(),
));
}
RenderError::VariableDoesNotExist { .. }
| RenderError::ArgumentDoesNotExist { .. } => {
return Err(VariableDoesNotExist::with_source_code(
Expand Down
Loading
Loading