Removes acl files from list of files to match when mapping url to file #1109

megoth · 2019-02-19T22:46:34Z

Should fix #1104

kjetilk

Looks good.

I'm thinking that it effectively does a greedy match, and I'm thinking it should be more modest... :-) In terms of a regexp, it did ^/index\..*/, and with this patch ^/index\..*[^\.acl]$/, but an alternative solution to this problem would have been ^/index.(.*?)/. In the latter case, it would have matched only one period after the filename, and so excluded the .acl, since that would have had two .s.

Not sure what would have been the best solution, though, so Approve.

rubensworks

I would propose to not hardcode the .acl suffix, as this would result in the same problem for .meta files. Another solution may be to configure an array for all blacklisted suffixes in the constructor.

kjetilk

Good point, @rubensworks . Yeah, I can see it being passed around as suffixAcl here and there. Doesn't seem to be very consistently applied, but I think it is a good idea to look into using that.

megoth · 2019-02-20T16:39:45Z

I'll push an update soon that doesn't hardcode it ^_^

megoth · 2019-02-20T17:04:56Z

@rubensworks @kjetilk I've pushed an update, hope it looks better ^_^ (Was uncertain how to include the knowledge of suffixes into resourceMapper, so hope the way I did it looks ok)

kjetilk

Yup, it does! @rubensworks ?

rubensworks · 2019-02-21T07:35:45Z

lib/resource-mapper.js

+
+    // Find a file with the same name (minus the dollar extension)
+    return (files.find(f => this._removeDollarExtension(f) === filename ||
+      (isIndex && f.startsWith(this._indexFilename + '.') && !f.endsWith(this._suffixes.acl))))


I would go one step further, and loop over all this._suffixes to exclude all file names that end with any of the defined suffixes. Right now, only index.html.acl will be blacklisted, but index.html.meta will still cause the same problem. A loop over this._suffixes would solve this.

For this purpose, it may be easier to refactor this._suffixes to be an array instead of a hash, as this is (AFAICS) only being used here.

megoth · 2019-02-28T16:48:22Z

@rubensworks then I've implemented your suggested changes (changing fileSuffixes from an object to an array). Could you have a look at the latest change, and see if you think it's good?

rubensworks

Looks good to me now! :-)

Perhaps the fileSuffixes variable name can be changed to something more descriptive such as blacklistedFileSuffixes, but I have no strong opinion on this.

RubenVerborgh

I'm missing unit tests for ResourceMapper here, as well as in #1067 actually. It is one of the only unit-testable classes we have, and it used to have 100% coverage with many cases. Could we keep that up?

RubenVerborgh · 2019-03-03T13:29:46Z

lib/resource-mapper.js

    indexFilename = 'index',
-    overrideTypes = { acl: 'text/turtle', meta: 'text/turtle' }
+    overrideTypes = { acl: 'text/turtle', meta: 'text/turtle' },
+    fileSuffixes = ['.acl', '.meta']


Unclear variable name. I would name it something like controlExtensions.

RubenVerborgh · 2019-03-03T13:30:12Z

lib/resource-mapper.js

    this._defaultContentType = defaultContentType
    this._indexFilename = indexFilename
    this._types = { ...types, ...overrideTypes }
+    this._suffixes = fileSuffixes


Idem. They're not just suffixes, they are suffixes for a specific kinds of files that needs special behavior.

RubenVerborgh · 2019-03-03T13:35:16Z

lib/resource-mapper.js

      }
    // Determine the path of an existing file
    } else {
      // Read all files in the corresponding folder


Outdated comment

RubenVerborgh · 2019-03-03T13:37:08Z

lib/resource-mapper.js

      // Find a file with the same name (minus the dollar extension)
-      let match = !searchIndex ? '' : (files.find(f => this._removeDollarExtension(f) === filename ||
-        (isIndex && f.startsWith(this._indexFilename + '.'))))
+      let match = searchIndex ? await this._getMatchingFile(folder, filename, isIndex) : ''


I'm not sure if I would move this specific bit to a new function. Is there potential for reuse?

My thinking was to make it more readable by giving it a name. Don't know of any potential reuse, no.

RubenVerborgh · 2019-03-03T13:41:19Z

lib/resource-mapper.js

+    const files = await this._readdir(folder)
+
+    // Find a file with the same name (minus the dollar extension)
+    return (files.find(f => this._removeDollarExtension(f) === filename ||


We should probably expand this condition across multiple lines now, given the increased complexity.

RubenVerborgh · 2019-03-03T13:44:06Z

lib/resource-mapper.js

+
+    // Find a file with the same name (minus the dollar extension)
+    return (files.find(f => this._removeDollarExtension(f) === filename ||
+      (isIndex && f.startsWith(this._indexFilename + '.') && !this._suffixes.some(suffix => f.endsWith(suffix)))))


Seems wasteful to always iterate over the extensions.

How about, in the constructor, we instead use a regex to determine whether something is a control file? Then the default value is /.(?:acl|meta)$/, and this routine just does this._controlFile.test(f).

That sounds reasonable - I'll write tests that make sure stuff works, and then try to optimize if per your suggestions 😸

megoth · 2019-03-03T16:24:24Z

Ah, yes, I've been lazy on tests here. I'll write them when I have time, hopefully during the next couple of days.

@RubenVerborgh

Trying to optimise code, and improve abstraction for better readability Hopefully responding to the concerns of @RubenVerborgh: #1109 (comment) and #1109 (comment)

megoth · 2019-03-07T16:58:41Z

@RubenVerborgh think I have addressed your concerns; I'm uncertain of how to run coverage reports on this actually, could you check? (And perhaps drop a line on how you do it here? Could read up more on it, but am in bit of a hurry right now =P )

RubenVerborgh

LGTM except for regexp.

RubenVerborgh · 2019-03-07T17:24:12Z

lib/resource-mapper.js

    this._defaultContentType = defaultContentType
    this._indexFilename = indexFilename
    this._types = { ...types, ...overrideTypes }
+    this._isControlFile = new RegExp(`.(?:${fileSuffixes.join('|')})$`)


Not the right regex: a) fileSuffixes already has a leading dot, so there are two dots now b) dots need to be escaped in regexes (but, since you are creating the regex from a string, actually needs double backslash '\\.).

Alternatively, you can just pass the regex itself instead of passing fileSuffixes.

That makes it difficult to configure the suffixes though =/

Hope the latest commit looks good.

Ryuno-Ki · 2019-03-07T18:53:05Z

If you're concerned about coverage... why not use a tool like CodeCov?
You could make it even part of the conditions to flag a PR as mergeable.

RubenVerborgh · 2019-03-07T20:20:37Z

lib/resource-mapper.js

    this._defaultContentType = defaultContentType
    this._indexFilename = indexFilename
    this._types = { ...types, ...overrideTypes }
+    this._isControlFile = new RegExp(`(?:${fileSuffixes.map(fs => fs.replace('.', '\\.')).join('|')})$`)


fs.replace(/\./g, '\\.') strictly, but good enough 😉

Regex is so difficult =P

RubenVerborgh · 2019-03-07T20:21:15Z

If you're concerned about coverage... why not use a tool like CodeCov?

Coverage in general is not great yet; but it was for this particular class 😎

Removes acl files from list of files to match when mapping url to file

b1562ac

Should fix #1104

megoth requested review from csarven, kjetilk and rubensworks February 19, 2019 22:46

kjetilk approved these changes Feb 19, 2019

View reviewed changes

rubensworks reviewed Feb 20, 2019

View reviewed changes

kjetilk requested changes Feb 20, 2019

View reviewed changes

Removing hardcoded string of .acl-suffix

218b36a

kjetilk approved these changes Feb 20, 2019

View reviewed changes

rubensworks suggested changes Feb 21, 2019

View reviewed changes

Now excluding both .acl- and .meta-files when finding default file

d3e6506

rubensworks approved these changes Mar 2, 2019

View reviewed changes

RubenVerborgh suggested changes Mar 3, 2019

View reviewed changes

Trying to optimize and improve readability through better abstraction

31f113a

Trying to optimise code, and improve abstraction for better readability Hopefully responding to the concerns of @RubenVerborgh: #1109 (comment) and #1109 (comment)

megoth requested a review from RubenVerborgh March 7, 2019 16:57

RubenVerborgh suggested changes Mar 7, 2019

View reviewed changes

Trying to fix regex

08684fb

megoth requested a review from RubenVerborgh March 7, 2019 18:19

RubenVerborgh approved these changes Mar 7, 2019

View reviewed changes

kjetilk merged commit 567b042 into release/v5.0.0 Mar 7, 2019

kjetilk deleted the fix/exclude-acl-in-url-mapping branch March 7, 2019 21:50

Removes acl files from list of files to match when mapping url to file #1109

Removes acl files from list of files to match when mapping url to file #1109

Uh oh!

Conversation

megoth commented Feb 19, 2019

Uh oh!

kjetilk left a comment

Choose a reason for hiding this comment

Uh oh!

rubensworks left a comment

Choose a reason for hiding this comment

Uh oh!

kjetilk left a comment

Choose a reason for hiding this comment

Uh oh!

megoth commented Feb 20, 2019

Uh oh!

megoth commented Feb 20, 2019

Uh oh!

kjetilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megoth commented Feb 28, 2019

Uh oh!

rubensworks left a comment

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megoth commented Mar 3, 2019

Uh oh!

megoth commented Mar 7, 2019

Uh oh!

RubenVerborgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ryuno-Ki commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants