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

where does not handle template.HTML and string comparisons #8353

Closed
richtera opened this issue Mar 22, 2021 · 18 comments · Fixed by #8461
Closed

where does not handle template.HTML and string comparisons #8353

richtera opened this issue Mar 22, 2021 · 18 comments · Fixed by #8461
Assignees
Milestone

Comments

@richtera
Copy link
Contributor

richtera commented Mar 22, 2021

$ hugo version
Hugo Static Site Generator v0.81.0-DEV-350E293E/extended linux/amd64 BuildDate: 2021-01-15T11:14:07-0500

Can be reproduced on latest, but I can't quite explain it.
I have these two range loops in my code and one works and the other doesn't.

  {{- range .Site.Pages -}}
    {{ $url := (.OutputFormats.Get "HTML").RelPermalink }}
    {{ if (eq .Params.name $shortUrl) }}
      {{ warnf "HasMatch1 %s" $url }}
      {{- $output.Set "match" $url -}}
    {{- end }}
  {{- end -}}
  {{- range where .Site.Pages "Params.name" $shortUrl -}}
    {{ $url := (.OutputFormats.Get "HTML").RelPermalink }}
    {{ warnf "HasMatch2 %s" $url }}
    {{- $output.Set "match" $url -}}
  {{- end -}}

It seems I only get outputs containing HasMatch1. Ideally, both should end up with the same items. What's curious is that most of the time "where" is working as expected and not causing problems. So I am not quite sure what's going on or how to debug it yet.

@jmooring
Copy link
Member

jmooring commented Mar 22, 2021

Which fails to produce the expected results?
a. The scratch
b. The warnf statement
c. Both

I am unable to find problems with either with a simple test.

@richtera
Copy link
Contributor Author

There are two loops. One using where and the other using if (eq) to compare the same path.
The version using if works and I see "hasMatch1" und the one using where doesn't return anything and I don't see "hasMatch2"
So even through Params.name for one of the pages matches, the where doesn't return that page.

@richtera
Copy link
Contributor Author

Found something interesting in

func (ns *Namespace) checkWhereArray(seqv, kv, mv reflect.Value, path []string, op string) (interface{}, error) {
	rv := reflect.MakeSlice(seqv.Type(), 0, 0)

	for i := 0; i < seqv.Len(); i++ {
		var vvv reflect.Value
		rvv := seqv.Index(i)

		if kv.Kind() == reflect.String {
			if params, ok := rvv.Interface().(maps.Params); ok {
				vvv = reflect.ValueOf(params.Get(path...))
			} else {
				vvv = rvv
				for _, elemName := range path {
					var err error
					vvv, err = evaluateSubElem(vvv, elemName)
					if err != nil {
                                                // THIS CONTINUE SHOULD REALLY CONTINUE THE OUTER LOOP
                                                // SO PROBABLY NEEDS SOME KIND OF ESCAPE JUST INCASE mv is null
						continue
					}
				}
			}
		} else {
			vv, _ := indirect(rvv)
			if vv.Kind() == reflect.Map && kv.Type().AssignableTo(vv.Type().Key()) {
				vvv = vv.MapIndex(kv)
			}
		}

		if ok, err := ns.checkCondition(vvv, mv, op); ok {
			rv = reflect.Append(rv, rvv)
		} else if err != nil {
			return nil, err
		}
	}
	return rv.Interface(), nil
}

But although the continue should be a break and then some kind of if nil continue outside of the path loop, this still would not explain the problem. Is is possible that somewhere along the way properties with the signature "name" are treated specially? Really don't know but when tracing through checkWhereArray I do not get a hit in checkCondition with path containing "Params", "name"

@moorereason
Copy link
Contributor

@richtera, can you provide a simple demo site the reproduces this issue?

@richtera
Copy link
Contributor Author

Ok, will try. I am debugging it currently. I can see it fail right in the middle of checkCondition.
Even though both sides are strings. mv.Type() == v.Type() returns false.
The left side (mv) for example is
image
The right side (v) is:
image
In the else part it immediately fails, because of

if mv.Kind() != reflect.Array && mv.Kind() != reflect.Slice {
  return false, nil
}

Switching

	if mv.Type() == v.Type() {
	if mv.Kind() == v.Kind() {

seems to repair the problem. I can make a PR if that makes sense.
I think the code is comparing Type() instead of Kind().
This is on line 90 of where.go.

@richtera
Copy link
Contributor Author

if mv.Type() == v.Type() {

@jmooring
Copy link
Member

jmooring commented Mar 23, 2021

I can reproduce the behavior by:

  1. Setting "name" in front matter to a slice (name = ["foo"])
  2. Setting $shortUrl in the OP's template code to a slice ($shortUrl := slice "foo")
git clone --single-branch -b hugo-github-issue-8353 https://github.com/jmooring/hugo-testing hugo-github-issue-8353
cd hugo-github-issue-8353
hugo

In this contrived example, where .Site.Pages "Params.name" $shortUrl is failing when both sides of the comparison are slices. But I'd expect this based on the documentation:
https://gohugo.io/functions/where/#use-where-with-intersect

When I use where .Site.Pages "Params.name" "intersect" $shortUrl I get the expected results.

Again, my failing test case is contrived, but I'm curious...

@richtera

  1. What is $shortUrl in your test? A string, int, slice?
  2. What is name in your front matter? A string, int, slice?

@richtera
Copy link
Contributor Author

richtera commented Mar 23, 2021

Params.name is a string from yaml in a md frontmatter.

(The snippet is running on a different page (a related page which is linked by the shortUrl)
$shortUrl is a local string variable using this hugo code:

  {{- $url := (.OutputFormats.Get "HTML").RelPermalink }}
  {{- $shortUrl := delimit ((slice "") | append (after 3 (split $url "/"))) "/" -}}

My RelPermalinks have 5 segments and $shortUrl only the last two. Since they are both strings it should have matched correctly, but I guess they are of different string Type() but the same Kind(). BTWL the image of the debugger of the two values further up in this thread shows that both use runtime.strequal for the comparator.

@jmooring
Copy link
Member

jmooring commented Mar 23, 2021

{{ $a := "foo/bar" }}
{{ printf "$a is type %T with value %#v" $a $a }}<br>

{{ $b := delimit (slice "foo" "bar") "/" }}
{{ printf "$b is type %T with value %#v" $b $b }}<br>

{{ if eq $a $b }}
  eq $a $b --> TRUE<br>
{{ end }}

{{ if in $a $b }}
  in $a $b --> TRUE<br>
{{ end }}

Produces:

$a is type string with value "foo/bar"
$b is type template.HTML with value "foo/bar"
eq $a $b --> TRUE
in $a $b --> TRUE 

About a year ago the last comparison (using the in function) would have been FALSE, but was fixed with #7005.

So I guess we need to do the same thing with where, or make delimit return a string.

@jmooring jmooring changed the title "Where" sometimes doesn't seem to work in does not work with template.HTML strings Mar 23, 2021
@jmooring jmooring changed the title in does not work with template.HTML strings where does not work with template.HTML strings Mar 23, 2021
@richtera
Copy link
Contributor Author

@jmooring Other than delimit, I do use printf to construct query strings for where; that might be another candidate if changing to comparing Kind() is not feasible.

@moorereason
Copy link
Contributor

The underlying issue is that we don't handle the case of comparing arrays and slices in checkCondition (the use case in this issue is comparing string slices). We setup the values to be compared here, but most of the basic comparisons don't test those values.

We probably need to pull out the core of the operations functions from tpl/compare and move them to common/compare. Then both tpl/compare and tpl/collections can use them. Any objections to that approach, @bep?

@moorereason moorereason changed the title where does not work with template.HTML strings where does not test arrays or slices for all operations Mar 23, 2021
@richtera
Copy link
Contributor Author

But my use case is a string to string comparison. There is no slice or duct involved. Hmmm.

@richtera
Copy link
Contributor Author

richtera commented Mar 23, 2021

@bep I am still concerned that this ticket got changed to talking about arrays and slices although my use case was using plain string comparison and failed because mv.Type() == v.Type() would return false and end up in the array/slice part of the comparison code.

@moorereason
Copy link
Contributor

Sorry, my research is based upon @jmooring's test site, whish may be presenting a slightly different use case. Do you have a demo site the reproduces your issue?

@richtera
Copy link
Contributor Author

I can create one. I just traced through the code and see the cause of the problem. Changing from Type() to Kind() makes everything work correctly. Ok I'll put one together.

@richtera
Copy link
Contributor Author

https://github.com/getselfstudy/hugo-where-failure
The index.html page will render although both range where should return the same page.
The range code is inside of navbar.html at layouts/partials/navbar.html
I hope the test is not too contrived.
It will also output two warnf showing the argument and result.

        <h1>TEST</h1>
        <ul>
            
            
            
                <li>Found /post/another-post/</li>
            
        </ul>
        <h1>TEST2</h1>
        <ul>
            
            
            
        </ul>

@moorereason
Copy link
Contributor

Sorry for the confusion. You're right about the template.HTML to string comparison. Joe's test site took me down the wrong (but also interesting) path. I'll fix the issue title.

The Type comparison in checkCondition has been there almost from the beginning for where (since v0.13 ... 6 years ago). You may be right about the fix. I'll look into it.

@moorereason moorereason changed the title where does not test arrays or slices for all operations where does not handle template.HTML and string comparisons Mar 23, 2021
@bep bep added the Bug label Mar 24, 2021
@bep bep added this to the v0.83 milestone Mar 24, 2021
@bep bep self-assigned this Apr 23, 2021
bep added a commit to bep/hugo that referenced this issue Apr 23, 2021
@bep bep closed this as completed in #8461 Apr 23, 2021
bep added a commit that referenced this issue Apr 23, 2021
@github-actions
Copy link

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants