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

Fix section index partial NPE #1890

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Fix section index partial NPE #1890

merged 1 commit into from
Apr 25, 2024

Conversation

tobiaskohlbau
Copy link
Contributor

During rendering the section index of a docs list it could happen that a file has no parent for e.g. the homepage. If such scenario is existing the .Parent.File of a file is nil. Therefore this case should be handled within the rendering itself.

Fixes #1874

Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

Just ran into the same issue, and the fix proposed with this PR solved my problem.
I realized that this contribution proposes a slight more elegant solution to the problem .
@tobiaskohlbau: can you have a look, please? Can we take over this solution here?

@tobiaskohlbau
Copy link
Contributor Author

tobiaskohlbau commented Mar 18, 2024

Just ran into the same issue, and the fix proposed with this PR solved my problem. I realized that this contribution proposes a slight more elegant solution to the problem . @tobiaskohlbau: can you have a look, please? Can we take over this solution here?

AFAIK the proposed solution would not work. As the and within the if only checks for the currently scoped Parent but not for every pages Parent.File.UniqueID. Therefore if pages do not have a Parent.File the access to UniqueID would throw a NPE. It's required to filter out all pages where Parent.File=nil. (IIRC I had such solution before and it still got said NPE.)

@yann-soubeyrand
Copy link
Contributor

Hello,

I confirm what @tobiaskohlbau said.

Using Hugo 0.124.1, which contains the fix referred to above, on a docs only site, that is with the following front matter in content/_index.adoc

title: 'Documentation'
layout: 'section'

cascade:
 - type: 'docs'

I still get a warning:

WARN  .File.UniqueID on zero object. Wrap it in if or with: {{ with .File }}{{ .UniqueID }}{{ end }}

But the warning is solved when adding the modified partial from this PR.

@yann-soubeyrand
Copy link
Contributor

I re-tested today and, in fact, this is not a warning, but an error:

ERROR render of "taxonomy" failed: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/docsy@v0.9.2-0.20240325160920-2e674ae180f3/layouts/docs/list.html:12:5": execute of template failed: template: docs/list.html:12:5: executing "main" at <partial "section-index.html" .>: error calling partial: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/docsy@v0.9.2-0.20240325160920-2e674ae180f3/layouts/partials/section-index.html:8:69": execute of template failed: template: partials/section-index.html:8:69: executing "partials/section-index.html" at <$parent.File.UniqueID>: error calling UniqueID: runtime error: invalid memory address or nil pointer dereference
Built in 3662 ms
Error: error building site: render: failed to render pages: render of "taxonomy" failed: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/docsy@v0.9.2-0.20240325160920-2e674ae180f3/layouts/docs/list.html:12:5": execute of template failed: template: docs/list.html:12:5: executing "main" – File is nil; wrap it in if or with: {{ with partial "section-index.html" .>: error calling partial: "/home/yann/.cache/hugo/modules/filecache/modules/pkg/mod/github.com/camptocamp/docsy@v0.9.2-0.20240325160920-2e674ae180f3/layouts/partials/section-index.html:8:69": execute of template failed: template: partials/section-index.html:8:69: executing "partials/section-index.html" at <.File }}{{ .UniqueID }}{{ end }}

I still confirm this PR fixes the issue.

@deining
Copy link
Collaborator

deining commented Apr 7, 2024

AFAIK the proposed solution would not work. As the and within the if only checks for the currently scoped Parent but not for every pages Parent.File.UniqueID. Therefore if pages do not have a Parent.File the access to UniqueID would throw a NPE. It's required to filter out all pages where Parent.File=nil. (IIRC I had such solution before and it still got said NPE.)

You are right. Your solution is more general and should be merged unchanged.

@deining deining added this to the 24Q2 milestone Apr 7, 2024
@deining deining mentioned this pull request Apr 7, 2024
21 tasks
@@ -4,7 +4,8 @@
{{ $pages = (where $pages "Type" "!=" "search") }}
{{ $pages = (where $pages ".Params.hide_summary" "!=" true) -}}
{{ $pages = (where $pages ".Parent" "!=" nil) -}}
{{ if and .Parent .Parent.File -}}
{{ $pages = (where $pages ".Parent.File" "!=" nil) -}}
{{ if and $parent $parent.File -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking at this more closely, it seems to make sense indeed.

Btw, the $parent variable is misnamed, because$parent is set to .Page.

Can we inline the variable instead?

Btw, IMHO .Page cannot be falsy, but .Page.File could be falsy. So the if condition can be simplified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

During rendering the section index of a docs list it could happen
that a file has no parent for e.g. the homepage. If such scenario is
existing the .Parent.File of a file is nil. Therefore this case should
be handled within the rendering itself.

Fixes google#1874

Signed-off-by: Tobias Kohlbau <tobias@kohlbau.de>
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

I'm willing to give this a try.
RSLGTM based on appraisals by @deining and @yann-soubeyrand.
Thanks all! ✨

@chalin chalin changed the title fix issue with section index partial Fix section index partial NPE Apr 25, 2024
@chalin chalin merged commit 3be4075 into google:main Apr 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE for $parent.File.UniqueID using Hugo 0.123.1.
4 participants