for #26: refactor: new getRequestEntity function #104
Conversation
|
Changes Unknown when pulling 6148e65 on refactor-filterRequest-more-26 into * on master*. |
|
Note: I set the coverage failure threshold low on coveralls (50%) so we can keep it on while doing incremental refactors and adding tests. As I refactor and add more coverage, I'll bump the coverage failure threshold up. |
6148e65 to
3909fe5
Compare
|
Changes Unknown when pulling 3909fe5 on refactor-filterRequest-more-26 into * on master*. |
|
|
||
| requestIsEntityResource = hostInEntity(entity.resources, requestTopHost) | ||
| if (requestIsEntityResource) { | ||
| requestEntityName = entityName |
There was a problem hiding this comment.
@groovecoder it seems that getRequestEntity is going to always search all the entityList before returning its result (even if a match has been already found), I'm wondering if the for loop could (or should) be exited earlier when we found the first match instead.
Other questions that come to my mind currently are:
- what is the expected result when no match has been found
- is it possible that more then one match is found for the request in the host list? if it is, what is the expected result?
There was a problem hiding this comment.
Great catch! Yes we can exit the for loop as soon as we find the first match. This code was moved from background.js where it was returning as soon as it found the first match.
When no match has been found, it needs to return an object with sameEntity: false, so calling code knows that getRequestEntity couldn't verify that the request is going to a host in the same entity as the origin.
There should not be multiple matches in the entityList - i.e., resource and property domains should only belong to a single entity. If there are for some reason, that would be a bug, but we should clean out dupes upstream from this code, so this code can return as quickly as possible while monitoring network requests. (I filed mozilla-services/shavar-list-creation#40)
I will add tests for these cases.
|
Changes Unknown when pulling 3f7fc32 on refactor-filterRequest-more-26 into * on master*. |
|
Updated. This will need a rebase/merge conflict resolution with #109 and this is probably lower priority since it's clean-up and the other is a launch-blocking UI update. |
|
Changes Unknown when pulling 587c979 on refactor-filterRequest-more-26 into * on master*. |
Also, tests and coverage reporting!
Return entityName: null if not found
587c979 to
ac45552
Compare
|
Changes Unknown when pulling ac45552 on refactor-filterRequest-more-26 into * on master*. |
@rpl - r?