Fix handling of taxonomy terms containing slashes #4388

moorereason · 2018-02-08T04:16:15Z

This PR attempts to resolve #4090. The first commit contains tests that demonstrate the problem.

Special thanks to @tsuereth for his initial work on #4092, which laid the groundwork for this PR.

This PR may fix the title issues in #3577, but I haven't tested it yet.

moorereason · 2018-02-08T22:02:56Z

@bep,
Can you take a look at this PR? I'm getting random test results when the race detector is enabled (on linux), which matches what Travis is showing (plus tip failed on darwin). Without the race detector, I can't get it to fail. For the life of me, I don't see what could be causing the failures.

I've confirmed the failures with Go 1.9.3, 1.9.4, 1.10rc1, and 1.10rc2.

The following will bypass the test cache in Go 1.10:

sh$ cd gohugoio/hugo/hugolib
sh$ while :; do echo `date` && go1.10rc2 test -race -run=TestTaxonomiesWithAndWithoutContentFile  ; done
Thu Feb 8 15:52:42 CST 2018
PASS
ok      github.com/gohugoio/hugo/hugolib        1.480s
Thu Feb 8 15:52:49 CST 2018
--- FAIL: TestTaxonomiesWithAndWithoutContentFile (0.00s)
    --- FAIL: TestTaxonomiesWithAndWithoutContentFile/uglyURLs=false,preserveTaxonomyNames=true (0.10s)
        Error Trace:    testhelpers_test.go:41
                        taxonomy_test.go:136
                        taxonomy_test.go:60
        Error:          Should be true
        Test:           TestTaxonomiesWithAndWithoutContentFile/uglyURLs=false,preserveTaxonomyNames=true
        Messages:       File no match for
                        "Cat1" in
                        "public/categories/cat1/index.html":
                        List|CAt1|
FAIL
exit status 1
FAIL    github.com/gohugoio/hugo/hugolib        0.551s
Thu Feb 8 15:52:51 CST 2018
--- FAIL: TestTaxonomiesWithAndWithoutContentFile (0.00s)
    --- FAIL: TestTaxonomiesWithAndWithoutContentFile/uglyURLs=true,preserveTaxonomyNames=true (0.10s)
        Error Trace:    testhelpers_test.go:41
                        taxonomy_test.go:136
                        taxonomy_test.go:60
        Error:          Should be true
        Test:           TestTaxonomiesWithAndWithoutContentFile/uglyURLs=true,preserveTaxonomyNames=true
        Messages:       File no match for
                        "Cat1" in
                        "public/categories/cat1.html":
                        List|CAt1|
    --- FAIL: TestTaxonomiesWithAndWithoutContentFile/uglyURLs=false,preserveTaxonomyNames=true (0.10s)
        Error Trace:    testhelpers_test.go:41
                        taxonomy_test.go:136
                        taxonomy_test.go:60
        Error:          Should be true
        Test:           TestTaxonomiesWithAndWithoutContentFile/uglyURLs=false,preserveTaxonomyNames=true
        Messages:       File no match for
                        "Cat1" in
                        "public/categories/cat1/index.html":
                        List|CAt1|
FAIL
exit status 1
FAIL    github.com/gohugoio/hugo/hugolib        0.468s

moorereason · 2018-02-09T01:35:12Z

Correction: I'm able to reproduce the anomaly in Go 1.10rc2 without the race detector.

moorereason · 2018-02-09T03:40:58Z

Fixed the issue. PTAL

bep

Looks solid, just a couple of minore comments.

bep · 2018-02-09T07:33:00Z

helpers/path.go

@@ -75,6 +75,35 @@ func (filepathBridge) Separator() string {

 var fpb filepathBridge

+// segmentReplacer replaces some URI-reserved characters in a path segments.
+var segmentReplacer = strings.NewReplacer("/", "-", "#", "-")


Why do you replace "-" with "-"? (the same character).

NewReplacer uses "old-new" pairs. This replaces '/' and '#' with '-'.

What if there is a ? character? Is it possible to use a common url encoding here?

See the tests below. The ? is removed. URL encoding is beyond the scope of this PR.

bep · 2018-02-09T07:36:54Z

helpers/path.go

+	var last byte
+	b := make([]byte, len(s))
+
+	for i := 0; i < len(s); i++ {


If I read this correctly this removes double-slashes. I assume that come from "My // Taxonomy" cases. Which is fine as long as this isn't used in the "general case" (which would break some URLs and make people grumpy). But you should add a test variant for it below.

Yes, we would replace both slashes with - using segmentReplacer (other interesting characters like a space and ? are replaced by MakePathSanitized used above), and this loop removes consecutive dashes.

So, My // Taxonomy would become my-taxonomy.

stale · 2018-06-24T15:05:02Z

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

moorereason · 2018-06-25T14:48:05Z

@bep,
It's been a while since I submitted this PR, but it's held up by your requested changes that I believe I've made. Can you add this to a future milestone for review? Thanks

onedrawingperday · 2018-08-20T16:29:53Z

@moorereason
Does your PR also apply to Taxonomies with custom permalinks configuration?

Let's say that I have a taxonomy called color and its permalinks are configured like so: color = "/:slug/"

Then the taxonomy is assigned to content files with: color = "handbags/backpacks/gold"

With the above Hugo is able to nest a taxonomy under a Section or another Taxonomy.

Also here is another use case with the above technique to generate Chronological Blog Archives.

As you understand in both use cases the forward slashes are desirable.

moorereason · 2018-08-20T18:29:41Z

@onedrawingperday, I think it will work as you expect, but I'd be more than happy to test a sample site if you have one available.

onedrawingperday · 2018-08-21T09:42:41Z

@moorereason Here is a sample repo with Nested Taxonomies for you to test.
https://github.com/onedrawingperday/hugo-test

I have included both of the use cases that I described above.
If you have a question let me know.

Thanks!

Add tests with slashes in terms to illuminate failing code paths. Updates gohugoio#4090

MakePathSegment properly sanitizes path segments (like taxonomy terms) with slashes (and pound signs). Updates gohugoio#4090

@tsuereth

This commit attempts to sanitize term keys and path segments to fix the handling of taxonomy terms containing slashes. Based upon gohugoio#4092 by @tsuereth. Fixes gohugoio#4090

Don't mix casing in the "cat1" taxonomy term test because Hugo doesn't support multiple terms whose lowercase strings collide. Related to gohugoio#4238

MakeSegment will replace slashes in a path segment, so make sure we include tests for that.

Case changed since last git rebase.

More cleanups from rebasing with master.

moorereason · 2018-08-21T14:53:56Z

Thanks, @onedrawingperday. I tested your site with my original PR branch and also rebased with the latest master. In both cases, your test site comes out exactly the same. 👍

github-actions · 2022-01-14T01:56:04Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

bep requested changes Feb 9, 2018

View reviewed changes

stale bot added the Stale label Jun 24, 2018

moorereason added Keep and removed Stale labels Jun 25, 2018

moorereason mentioned this pull request Jul 12, 2018

# not escaped in Permalink when in title value #4926

Closed

This was referenced Jul 20, 2018

fix: tag include "#" Special character show 404 xianmin/hugo-theme-jane#84

Closed

Terms (Tags/Categories) output incorrect/broken directory names #4519

Closed

moorereason added 7 commits August 21, 2018 08:52

hugolib: Add taxonomy tests with terms containing a slash

56a8aa1

Add tests with slashes in terms to illuminate failing code paths. Updates gohugoio#4090

helpers: Add PathSpec.MakePathSegment

574d7a8

MakePathSegment properly sanitizes path segments (like taxonomy terms) with slashes (and pound signs). Updates gohugoio#4090

hugolib: Remove useless assignment in newTaxonomyPage

a890d8a

hugolib: Fix handling of taxonomy terms containing slashes

85f50e0

This commit attempts to sanitize term keys and path segments to fix the handling of taxonomy terms containing slashes. Based upon gohugoio#4092 by @tsuereth. Fixes gohugoio#4090

hugolib: Revert taxonomy test change

58b6678

Don't mix casing in the "cat1" taxonomy term test because Hugo doesn't support multiple terms whose lowercase strings collide. Related to gohugoio#4238

helpers: Add double-slash test for MakeSegment

a779231

MakeSegment will replace slashes in a path segment, so make sure we include tests for that.

Fix case of PathSpec.DisablePathToLower

3fb91dd

Case changed since last git rebase.

moorereason force-pushed the iss4090 branch from 4ead589 to 3fb91dd Compare August 21, 2018 13:54

Fix TestMakeSegment

1253d8a

More cleanups from rebasing with master.

bep merged commit fff1325 into gohugoio:master Aug 22, 2018

onedrawingperday mentioned this pull request Sep 18, 2018

Commit 06976eb: broke Taxonomies with custom permalinks configuration #5223

Closed

moorereason deleted the iss4090 branch December 17, 2020 18:20

github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of taxonomy terms containing slashes #4388

Fix handling of taxonomy terms containing slashes #4388

moorereason commented Feb 8, 2018 •

edited

Loading

moorereason commented Feb 8, 2018

moorereason commented Feb 9, 2018

moorereason commented Feb 9, 2018

bep left a comment

bep Feb 9, 2018

moorereason Feb 9, 2018

Zebradil Jul 20, 2018

moorereason Jul 20, 2018

bep Feb 9, 2018

moorereason Feb 9, 2018

moorereason Feb 24, 2018

stale bot commented Jun 24, 2018

moorereason commented Jun 25, 2018

onedrawingperday commented Aug 20, 2018 •

edited

Loading

moorereason commented Aug 20, 2018

onedrawingperday commented Aug 21, 2018 •

edited

Loading

moorereason commented Aug 21, 2018

github-actions bot commented Jan 14, 2022

Fix handling of taxonomy terms containing slashes #4388

Fix handling of taxonomy terms containing slashes #4388

Conversation

moorereason commented Feb 8, 2018 • edited Loading

moorereason commented Feb 8, 2018

moorereason commented Feb 9, 2018

moorereason commented Feb 9, 2018

bep left a comment

Choose a reason for hiding this comment

bep Feb 9, 2018

Choose a reason for hiding this comment

moorereason Feb 9, 2018

Choose a reason for hiding this comment

Zebradil Jul 20, 2018

Choose a reason for hiding this comment

moorereason Jul 20, 2018

Choose a reason for hiding this comment

bep Feb 9, 2018

Choose a reason for hiding this comment

moorereason Feb 9, 2018

Choose a reason for hiding this comment

moorereason Feb 24, 2018

Choose a reason for hiding this comment

stale bot commented Jun 24, 2018

moorereason commented Jun 25, 2018

onedrawingperday commented Aug 20, 2018 • edited Loading

moorereason commented Aug 20, 2018

onedrawingperday commented Aug 21, 2018 • edited Loading

moorereason commented Aug 21, 2018

github-actions bot commented Jan 14, 2022

moorereason commented Feb 8, 2018 •

edited

Loading

onedrawingperday commented Aug 20, 2018 •

edited

Loading

onedrawingperday commented Aug 21, 2018 •

edited

Loading