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

Fixed #35571 -- Store template_names as string #18331

Closed
wants to merge 1 commit into from

Conversation

guserav
Copy link

@guserav guserav commented Jul 1, 2024

Trac ticket number

ticket-35571

Branch description

When passing pathlib.Path or pathlib.PosixPath as template names (because templates are in subdirectories, so pathlib seems reasonable to use) the assertTemplateUsed breaks as it performs a str.join on all template names without sanitizing them.

File "<stuff>/.local/lib/python3.12/site-packages/django/test/testcases.py", line 681, in _assert_template_used                                                                                                                      
    % (template_name, ", ".join(template_names)),                                                                                                                                                                                              
                      ^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                                                                
TypeError: sequence item 0: expected str instance, PosixPath found

This causes a lot of confusion as it is quite hard to track down the Template that introduced the PosixPath.

This is one proposed solution for fixing this. I.e. trying to sanitize all template names when they are passed into the template engine, as this seems easier then trying to find all cases where the stringiness of the template name is assumed. Another approach would be to have an assert that the template name is a string, but this could possible break code that doesn't utilize the test framework.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

When passing pathlib.Path or pathlib.PosixPath as template names (because templates are in subdirectories, so pathlib seems reasonable to use) the assertTemplateUsed breaks as it performs a str.join on all template names without sanitizing them.

```
File "<stuff>/.local/lib/python3.12/site-packages/django/test/testcases.py", line 681, in _assert_template_used
    % (template_name, ", ".join(template_names)),
                      ^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: sequence item 0: expected str instance, PosixPath found
```

This causes a lot of confusion as it is quite hard to track down the Template that introduced the PosixPath.

This is one proposed solution for fixing this. I.e. trying to sanitize all template names when they are passed into the template engine, as this seems easier then trying to find all cases where the stringiness of the template name is assumed. Another approach would be to have an assert that the template name is a string, but this could possible break code that doesn't utilize the test framework.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@nessita
Copy link
Contributor

nessita commented Jul 1, 2024

Hello @guserav, thank you for your first contribution! 🌟

As I mentioned in the ticket, we would need to first fully understand the report by having a minimal reproducer or a Django test case, for then to be able to accept the ticket and thus properly reviewing this PR.
With the information at hand, I have to close this since the proposed solution feels like it could impact performance for no clear reason/gain. This would also need tests and some extra docs changes, but first we need to understand the actual problem to decide on the right fix.

@nessita nessita closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants