Add a way for the user to control errors in resources.Get

[bep]

See https://discourse.gohugo.io/t/0-90-0-how-to-check-for-local-resource-existence/35954

In getJSON we have a rather intricate setup where we allow users to "ignore" HTTP errors.

I'm not a big fan of this construct, but we probably need a better way to allow the users handle this themselves.

We have talked about this before, but since then we have better control of the truth function in Go templates, so it should be able to do something ala:

{{ $image := with resources.Get "https://example.org/image.jpg }}
{{ with $image }}
{{ else }}
{{ with $image.Error }}
// Ignore or log
{{ end 
{{ end }}

I'm not totally sure if the above is a great idea or not ...

/cc @jmooring @regisphilibert etc.

[regisphilibert]

So even though we have $image.Error, $image alone would hold a falsy value?

[bep]

$image alone would hold a falsy value?

That's the general idea, yes.

[regisphilibert]

I think being able to interpret the error is always great and that's been lacking in getJSON etc... So I would, for one, love to be able to read the error when it happens, and decide what to do.

I'm not sure I trust $image value to test the success of my "fetch". It could very well return an empty array. I wonder if we couldn't add a OK or .Success in there so the user can choose to trust the falsy/truthy value of $image or rely on a more trustworthy $image.OK. This might not be very useful here, but if this was added to getJSON it would have value I believe.

{{ $image := with resources.Get "https://example.org/image.jpg }}
{{ with $image.OK }}
  // deal with $image's value even if empty array
{{ else }}
  {{ with $image.Error }}
  // Ignore or log
  {{ end 
{{ end }}

[bep]

@regisphilibert hmm... that's probably more added complexity than I'm prepared to take on.

I have thought a little, and suggest:

{{ $image := with resources.Get "https://example.org/image.jpg }}
{{ with $image }}
// This means we either have a valid image or an Error (any remote 404 means a nil value, aka not found)
// If you do this and there is an error situation, you will get an ERROR in the log (this is more or less how it behaves today)
{{ .RelPermalink }}
// To handle that error you would do something ALA:
{{ if .Error }}
{{ else }}
{{ .RelPermalink }}
{{ end }}
{{ end }}

[jmooring]

Notes on failure modes/delays...

This works:

{{ resources.Get "https://gohugo.io/img/hugo-logo.png" }}

This fails relatively quickly:

{{ resources.Get "https://gohugo.io/xxx/hugo-logo.png" }}

error calling Get: failed to retrieve remote resource: Not Found
Total in 4852 ms

This also fails relatively quickly:

{{ resources.Get "https://xxx.gohugo.io/img/hugo-logo.png" }}

error calling Get: Get "https://xxx.gohugo.io/img/hugo-logo.png": dial tcp: lookup xxx.gohugo.io: no such host
Total in 4095 ms

This takes a really long time to fail:

{{ resources.Get "https://gohugo-xxx.io/img/hugo-logo.png" }}

error calling Get: Get "https://gohug-xxxx.io/img/hugo-logo.png": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
Total in 44023 ms

[jmooring]

@bep Would your suggestion apply to all methods for both global and page resources? Thinking of:

• Add a ResourceGetter interface with Get,GetMatch,Match and make sure both .Resources and resources implements it, ByType #8653
• resources: Create a ResourceFinder interface #9063

I like your suggestion. To fail quietly:

{{ with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
  {{ if not .Error }}
    <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
  {{ end }}
{{ end }}

[vanbroup]

What if with (or a new equivalent) would handle errors as 'absent or empty' as described in the docs:

Rebinds the context (.) within its scope and skips the block if the variable is absent or empty.

This way the user can simply do:

{{ with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
    <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
{{ end }}

If the function could return the error, the user can handle the error if desired:

{{ $err := with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
    {{ if $err }}
        Failed to load image {{ $err }}
    {{ else }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
    {{ end }}
{{ end }}

[bep]

If the function could return the error, the user can handle the error if desired:

@vanbroup I cannot wrap my head around any implementation of your suggestion. Could you somehow outline that? Most importantly how the return value from resources.Get can be falsy and still a valid Resource. I understand it can be implemented, but that would surely break a lot of sites in the wild, and I'm not totally sure it would make a good API.

This takes a really long time to fail:

Yea, that is an unfortunate side effect of a bug in Go see golang/go#49366 -- It's fixed in 1.17.4, so I'll created #9265 to track that fix. Let's try to get these 2 issues fixed in 0.90.1.

As to the API, we also need to consider complexity/implementation cost.

So, to be more specific, what I suggest is:

We add a new method Err() error to the core Resource interface (the naming is in line with Go conventions). Which is pretty much implemented by everything in Hugo. We should try to get positional information into this, but that may be hard, so let's take that next.

And, as long as this doesn't blow the performance to pieces (run benchmarks; if that doesn't work, we may have to create a wrapper), we should be able to inject a check here:

hugo/tpl/tplimpl/template_funcs.go

Line 109 in 3d5dbdc

1. Check if receiver implements Err() error interface and the method in play is != Err.
2. If yes, invoke err := Err(); if err != nil { panic(err) }

And then make sure to set and return the err when applicable.

EDIT: Scratch the reflection magic above, I suggest we just create a resourceErr wrapper (which implements the Resource interface(s)) and return that in every error situation.

With all of that in place:

This will work:

{{ with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
  {{ if not .Err }}
    <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
  {{ end }}
{{ end }}

And this will be (mostly) backwards compatible:

{{ with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
    <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
{{ end }}

As in: If resources.Get fails, it will fail on first method invocation on that Resource, .RelPermalink in the example above.

We should also as part of this double check that we only return errors for real errrors (not 404).

[vanbroup]

@vanbroup I cannot wrap my head around any implementation of your suggestion. Could you somehow outline that? Most importantly how the return value from resources.Get can be falsy and still a valid Resource. I understand it can be implemented, but that would surely break a lot of sites in the wild, and I'm not totally sure it would make a good API.

The with function is a method to check that the context (object) is not absent or empty, if there is an error it does not exists, so it's absent, and strange that it would execute the with conditioned block. The logic for with could check if the context implements Err() as you stated before.

But then you would still need a way to check for errors. In my example above, I stated $err := with ... which might not make sense as on error the with block should not be executed and you would not have access to the error.

If the user wanted to check for an error, they could instead avoid using with and do the following:

{{ $img := resources.Get "https://gohugo.io/img/hugo-logo.png" }}
  {{ if not $img.Err }}
    <img src="{{$img .RelPermalink }}" width="{{$img .Width }}" height="{{ $img.Height }}" alt="">
  {{ end }}
{{ end }}

We should also as part of this double check that we only return errors for real errrors (not 404).

Why would a 404 HTTP status not be an error? The resource would not be available?

[bep]

Why would a 404 HTTP status not be an error? The resource would not be available?

1. Because it says so in the interface specification.
2. It makes sense -- as it is now the user cannot do if remote image does not exist, fall back to local etc.

[regisphilibert]

I suppose this is out of the question in Go Template?

{{ $err, $resource := resources.Get "https://gohugo.io/img/hugo-logo.png" }}

[bep]

I suppose this is out of the question in Go Template?

Yes.

[vanbroup]

I suppose this is out of the question in Go Template?

{{ $err, $resource := resources.Get "https://gohugo.io/img/hugo-logo.png" }}

It would break the current Hugo API as widely used, but this is possible in Go templates.

[vanbroup]

Why would a 404 HTTP status not be an error? The resource would not be available?

1. Because it says so in the interface specification.
2. It makes sense -- as it is now the user cannot do if remote image does not exist, fall back to local etc.

That is why I think with should never return an error but skip the block. But if you do check for errors ALL errors should return an error.

[bep]

It would break the current Hugo API as widely used, but this is possible in Go templates.

I'm pretty sure that it's not possible in Go templates, see https://go.dev/play/p/FAFc7LdJcy3

[bep]

@vanbroup re this:

{{ $err := with resources.Get "https://gohugo.io/img/hugo-logo.png" }}
    {{ if $err }}
        Failed to load image {{ $err }}
    {{ else }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
    {{ end }}
{{ end }}

with is a keyword and not a function, it does not return anything. We could make the Error falsy and say:

{{ $result := resources.Get "https://gohugo.io/img/hugo-logo.png" }}
{{ with $result }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
{{ else }}
// $result may be an error, or nil. We could create a template func to check, e.g.
{{ if reflect.IsError $result }}
{{ end }}
{{ end }}

But the above would be a breaking change, and I'm not sure I would call it prettier.

[vanbroup]

I'm pretty sure that it's not possible in Go templates, see https://go.dev/play/p/FAFc7LdJcy3

hmmm, got confused by the range, never noticed this...

[vanbroup]

{{ if reflect.IsError $result }}

I don't think we want to use reflect, what if $result would be an empty resource who implements the error?

So:

{{ $result := resources.Get "https://gohugo.io/img/hugo-logo.png" }}
{{ with $result }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
{{ else  }}
        {{ $result.Err }} // If $result is not a resource it's an error 
{{ end }}

I do not think this would break anything as {{ with $result }} would still only succeed when a resource is returned but I don't know if the with keyword can implement this logic.

[bep]

I do not think this would break anything as {{ with $result }} would still only succeed when a resource is returned but I don't know if the with keyword can implement this logic.

So, it would be possible to implement, but it would break in the error cases (as in: Before this change the build would break on any error, after this change it would be up to the user to handle all errors.

[regisphilibert]

I'll just note that while fetching media (images, fonts, css) is an interesting use case, I believe this will become the new getJSON for many users. And as such error handling is very welcome.

Now, I just want to make sure of one thing and pardon if I'm not seeing the obvious here. Could a working response (no http error) return a falsy value like an empty string.

If so, wouldn't with fail and yet $result.Err be inexistent?

[vanbroup]

I do not think this would break anything as {{ with $result }} would still only succeed when a resource is returned but I don't know if the with keyword can implement this logic.

So, it would be possible to implement, but it would break in the error cases (as in: Before this change the build would break on any error, after this change it would be up to the user to handle all errors.

Agreed, this is a 'breaking' change, does this have an impact on any users?

Currently it would panic and stop, the user would need to fix. Should users not only run into these types of issues during changes/development? With remote resources this is much more likely to happen and could break your site unexpectedly.

When using with you basically state that you expect that it might not be there which would be my argument to state this is not a breaking change.

[bep]

Should users not only run into these types of issues during changes/development?

What if a remote server is down when building your site on some CI server?

The thing is, this is very contextual. I would say that in most situations, the current behaviour of "stop the build as fast as possible" is the best thing to do. But there are exceptions -- and if you have a fragile remote service somewhere it may make sense to just drop that content or display some default content...

The thing is, I would possibly consider a breaking change like this if it was cleaner/easier/simpler/better (and preferably much) to implement:

Compare these two:

{{ $result := resources.Get "https://gohugo.io/img/hugo-logo.png" }}
{{ with $result }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
{{ else  }}
        {{ $result.Err }} // If $result is not a resource it's an error 
{{ end }}

{{ $result := resources.Get "https://gohugo.io/img/hugo-logo.png" }}
{{ with $result }}
        {{ if .Err }}
        {{ else }}
        <img src="{{ .RelPermalink }}" width="{{ .Width }}" height="{{ .Height }}" alt="">
        {{ end }}
{{ end }}

The latter example is fully backwards compatible**, it's a pattern that is fairly simple to implement (no trickery with the truth function used in with) and it's fairly easy understand and expand to the other APIs we have, if needed.

** Breakages in the first example: All errors must be handled by the user, $result cannot be compared to nil.

[vanbroup]

I think we can go in the direction you suggested, it requires a bit more conditions in some scenarios but that is not a big deal.

The 404 case is still an interesting one, we need to be backwards compatible but if you try to load data from a remote server and get a 404, the data will not load but you will also not see any errors because those are explicitly ignored.

You could cause the build to fail using errorf as result of a nil result, but I would expect an error like as would happen for all other errors. This might need to clearly addressed in the documentation!

[bep]

Fixed in e4d6ec9

[github-actions]

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.