Problems with internal link cache re-checking logic (4.0.0.rc3) #695

riccardoporreca · 2022-01-09T00:18:16Z

Cached internal broken links are not re-checked

rm -rf example_site
mkdir -p example_site
echo '
<!doctype html>
<html>
<head><title>Example</title></head>
<body>
<a href="/missing.html">Missing internal link</a>
</body>
</html>
' > example_site/index.html

Running first with empty cache

rm -rf tmp/.htmlproofer
bundle exec htmlproofer example_site --log-level debug --cache '{"timeframe": "1d"}'
# [...]
# Adding /missing.html to internal cache
# Adding 1 new internal link to the cache
# Removing 0 outdated internal links from the cache
# Checking 1 internal link
# Ran on 1 file!
#
#
# For the Links > Internal check, the following failures were found:
#
# * In example_site/index.html (line 5):
#
#   internally linking to /missing.html, which does not exist
#
#
# HTML-Proofer found 1 failure!

Run again: the broken link is not re-checked

rm -rf tmp/.htmlproofer
bundle exec htmlproofer example_site --log-level debug --cache '{"timeframe": "1d"}'
# Found 1 internal link in the cache
# Adding 0 new internal links to the cache
# Removing 0 outdated internal links from the cache
# Checking 0 internal links
# Ran on 1 file!
#
#
# HTML-Proofer finished successfully.

riccardoporreca · 2022-01-09T00:22:40Z

I have been investigating this, which is primarily caused by overriding the metadata, which loses the found information (confirmed by found being null in cache.json after the second round above)

html-proofer/lib/html_proofer/cache.rb

Lines 97 to 111 in 8107f0c

    
           additions = urls_detected.reject do |url, metadata| 
        
             if @cache_log[type].include?(url) 
        
               @cache_log[type][url][:metadata] = metadata 
        
               # if this is false, we're trying again 
        
               if type == :external 
        
                 @cache_log[type][url][:found] 
        
               else 
        
                 @cache_log[type][url][:metadata].none? { |m| m[:found] } 
        
               end 
        
             else 
        
               @logger.log :debug, "Adding #{url} to #{type} cache" 
        
               false 
        
             end 
        
           end

I am trying to propose a fix following my investigations

* Reveals the issue gjtorikian#695 with checking failed links * Also fix test cache for within-date checking

…torikian#695) * We cannot overwrite the cache metadata before checking if the internal link was found. * Don't recheck only if all cached metadata references to an internal link passed. * Log the re-addition of not found urls (debug level)

riccardoporreca · 2022-01-09T00:53:03Z

See PR #696

Note that there is still one main flaw even after the proposed fix, which re-checks an internal url if not all cached checks were OK:

html-proofer/lib/html_proofer/cache.rb

Line 104 in 287bd2d

@cache_log[type][url][:metadata].all? { |m| m[:found] }

This is not enough, since a relative URL that was OK for all the files previously checked and cached does not provide any guarantee against e.g. the same URL being OK for a new file (for instance, this could happen if a file is moved and the relative urls in it would not be updated). This is very related to #671 (comment)

…ache * See gjtorikian#695 (comment) * The link should be re-checked as it was not checked for the given file, regardless of whether it was found OK in the cache for other locations. * This is currently failing, as we miss the logic for determining this should be re-checked.

riccardoporreca · 2022-01-09T01:03:45Z

I have created a new branch off the one of PR #696, where I could add a test for the problem mentioned in #695 (comment) above

…ache * See gjtorikian#695 (comment) * The link should be re-checked as it was not checked for the given file, regardless of whether it was found OK in the cache for other locations. * This is currently failing, as we miss the logic for determining this should be re-checked.

…data * See gjtorikian#695 (comment) * The new combined key prevents using the metadata to store the information, and uniquely identifies what is relevant for checking internal links. * This aligns internal and external collection and cache. * The combined key needs to be serialized to a string when writing the cache to JSON and parsed when reading.

* To show the issue mentioned in gjtorikian#695 (comment) is addressed with the new approach to internal cache keys.

riccardoporreca · 2022-01-09T02:11:37Z

@gjtorikian, I took a bit of liberty (and fun) to investigate a possible rethinking of internal links cache keys, by using a combined key including the relevant "context" used for checking them. This was also largely inspired by your considerations in #671 (comment) and #671 (comment)

You can find this drafted in a branch: riccardoporreca/html-proofer@feature/695-fix-internal-cache-metadata-check...riccardoporreca:feature/695-investigate-internal-cache-key-metadata

The approach seems to work, although it has downsides in the JSON cache, where keys become less readable.
The failing tests are only for the cache, since the test expectations and data are not aligned. I only aligned the newly-introduced test revealing the issue.

* To show the issue mentioned in #695 (comment) is addressed with the new approach to internal cache keys.

gjtorikian · 2022-01-09T17:52:33Z

The approach seems to work, although it has downsides in the JSON cache, where keys become less readable.
The failing tests are only for the cache, since the test expectations and data are not aligned. I only aligned the newly-introduced test revealing the issue.

Gotcha. Thanks for the explanation. I'm far less concerned about a human-readable version of the cache.

gjtorikian · 2022-01-09T18:01:02Z

I need to look at this one a bit more carefully, because I bungled up your branch with a force push. There are a couple of tests still failing but in general I think what you've done makes sense.

riccardoporreca · 2022-01-09T22:30:45Z

There are a couple of tests still failing but in general I think what you've done makes sense.

Indeed, I did not want to fix tests before knowing if the approach would be something workable at your end. I think it is important to align all test cache files to the new structure: tests checking if a link is added because it was failed in the cache or the cache expired might pass for the wrong reason of the key not being in the cache

Let me know if I can help with this...

gjtorikian · 2022-01-28T00:57:17Z

I just remembered I dropped this—will try to make time for it this weekend.

* Reveals the issue #695 with checking failed links * Also fix test cache for within-date checking

riccardoporreca added a commit to riccardoporreca/html-proofer that referenced this issue Jan 9, 2022

Add tests for failed internal links re-check

5a80f7c

* Reveals the issue gjtorikian#695 with checking failed links * Also fix test cache for within-date checking

riccardoporreca mentioned this issue Jan 9, 2022

Fix internal cache metadata check #696

Merged

riccardoporreca added a commit to riccardoporreca/html-proofer that referenced this issue Jan 9, 2022

Align test for newly-broken internal link

ec99240

* To show the issue mentioned in gjtorikian#695 (comment) is addressed with the new approach to internal cache keys.

gjtorikian closed this as completed in #696 Jan 9, 2022

gjtorikian reopened this Jan 9, 2022

gjtorikian mentioned this issue Jan 9, 2022

Restore pr 696 #697

Closed

gjtorikian pushed a commit that referenced this issue Jan 9, 2022

Align test for newly-broken internal link

a2b07ec

* To show the issue mentioned in #695 (comment) is addressed with the new approach to internal cache keys.

gjtorikian pushed a commit that referenced this issue Feb 16, 2022

Add tests for failed internal links re-check

14e581d

* Reveals the issue #695 with checking failed links * Also fix test cache for within-date checking

gjtorikian mentioned this issue Feb 17, 2022

Fix internal link format #699

Merged

gjtorikian closed this as completed in #699 Mar 10, 2022

riccardoporreca mentioned this issue Mar 14, 2022

Issue with internal links re-checking logic / cache keys #702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problems with internal link cache re-checking logic (4.0.0.rc3) #695

Problems with internal link cache re-checking logic (4.0.0.rc3) #695

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022 •

edited

Loading

riccardoporreca commented Jan 9, 2022

gjtorikian commented Jan 9, 2022

gjtorikian commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

gjtorikian commented Jan 28, 2022

Problems with internal link cache re-checking logic (4.0.0.rc3) #695

Problems with internal link cache re-checking logic (4.0.0.rc3) #695

Comments

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022 • edited Loading

riccardoporreca commented Jan 9, 2022

gjtorikian commented Jan 9, 2022

gjtorikian commented Jan 9, 2022

riccardoporreca commented Jan 9, 2022

gjtorikian commented Jan 28, 2022

riccardoporreca commented Jan 9, 2022 •

edited

Loading