967196 - Tagging service should trim tags

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

There's no valid reason to not do that, we are trimming in backups code, but the tags backend should take care of that, though we need a schema migration to fixup existing tags and some tests.

Comment 1

[Chirag Bhatia]

Hi Marco,

I would like to work on this bug. Can you please tell me how to reproduce this and how I can go about to fixing it? (if you are the mentor for this bug)

Comment 2

[Marco Bonardo [:mak]]

There is no way in the UI to reproduce because we trim in the calling code, though you can use this code snippet

PlacesUtils.tagging.tagURI(makeURI("http://www.example.com/"), [ " test " ]); PlacesUtils.tagging.getTagsForURI(makeURI("http://www.example.com"));

This might also be useful for a unit test, you can add a test to toolkit/components/places/tests/unit/test_tagging.js

Comment 3

[Marco Bonardo [:mak]]

the code is in nsTaggingService.js, we need to trim in tagURI and issue a Deprecate.warning if one passes an untrimmed tag to untagURI or getURIsForTag

Comment 4

[Chirag Bhatia]

Attachment: file.txt

Ok,

Based on your input in Comment 3, I've made some changes in nsTaggingService.js (file attached), can you please tell me if I'm in the right direction? Also, I'm not sure what you mean by issuing a Deprecate.warning, I searched mxr for similar keywords in the code but couldn't find any.

And where do I put the code in Comment 2 to test it? Is there any process I need to use to know to execute and use the unit test file? Or do I just enter the code in the JavaScript console for testing it?

Comment 5

[Marco Bonardo [:mak]]

(In reply to Chirag Bhatia from comment #4)
Also, I'm not sure what you mean by issuing a
> Deprecate.warning, I searched mxr for similar keywords in the code but
> couldn't find any.

Oh yeah it was a typo, it's Deprecated.warning from Deprecated.jsm module
mxr.mozilla.org/mozilla-central/search?string=Deprecated.warning

> And where do I put the code in Comment 2 to test it? Is there any process I
> need to use to know to execute and use the unit test file? Or do I just
> enter the code in the JavaScript console for testing it?

As I said you can modify the test_tagging.js test in toolkit/components/places/tests/unit/test_tagging.js
To run this test you can use
./mach xpcshell-test toolkit/components/places/tests/unit/test_tagging.js

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8586914 [details] [diff] [review]
file.txt

Review of attachment 8586914 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsTaggingService.js
@@ +133,5 @@
>    tagURI: function TS_tagURI(aURI, aTags)
>    {
> +    for (tag in aTags) {
> +      tag = tag.trim();
> +    }

Here you still don't know if aTags is a valid array... you should move this later after _convertInputMixedTagsArray has done input validation

You should use use some Array helper like .map() to apply a function to each entry in the array

@@ +209,5 @@
> +    for (tag in aTags) {
> +      if (tag != tag.trim()) {
> +        //throw Deprecate.Warning();
> +      }
> +    }

Again you should do later, when you know aTags is a valid array of tags.
And I think you can detect trailing or prefix spaces using a regular expression that will match whitespaces at the beginning or end. You can use the .some() helper from Array (note we want to issue a warning only once, not once per tag)

@@ +246,5 @@
>    // nsITaggingService
>    getURIsForTag: function TS_getURIsForTag(aTagName) {
> +    if (aTagName != aTagName.trim()) {
> +      // throw Deprecate.Warning();
> +    }

ditto, move it after input validation and the same as untagURI (apart here you don't need to use .some)

Comment 7

[Chirag Bhatia]

Attachment: file.txt

Ok, I made the changes as per your previous comment. Let me know if it looks correct so that I can go ahead with the testing.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8587602 [details] [diff] [review]
file.txt

please mark the attachment as "patch" and use the feedback? flag on it (it's better for attachments than needinfo)

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8587602 [details] [diff] [review]
file.txt

Review of attachment 8587602 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsTaggingService.js
@@ +139,5 @@
>      // This also does some input validation.
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
> +    aTags = aTags.map(function(tag) {
> +      return tag.trim()

Why are you using aTags? tags is a normalized and cleaned up version of it, and later we use tags, not aTags.

I think you can compact this code too
tags = tags.map(tag => tag.trim());

@@ +219,5 @@
>      // This also does some input validation.
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
> +    let isAnyTagNotTrimmed = aTags.some(function(tag) {
> +      return (tag.match(/^\s/) || tag.match(/\s$/))

this way you are creating 2 regex for each tag, and you are also using .match, but we don't need matches.
that is not efficient. First you can use .test() on regex, that will give you a bool.
then you can build a single regex like /(^\s|\s$)/ outside of the function and reference it inside (so it is compiled only once).

@@ +223,5 @@
> +      return (tag.match(/^\s/) || tag.match(/\s$/))
> +    });
> +
> +    if (isAnyTagNotTrimmed) {
> +      Deprecated.warning("Atleast one tag passed in untagURI was not trimmed");

this method requires passing a url as second parameter, you can use the url of this bug report for it.

Also typo: Atleast => At least
and passed in => passed to

@@ +250,5 @@
>    getURIsForTag: function TS_getURIsForTag(aTagName) {
>      if (!aTagName || aTagName.length == 0)
>        throw Cr.NS_ERROR_INVALID_ARG;
>  
> +    if (aTagName.match(/^\s/) || aTagName.match(/\s$/)) {

ditto, use .test and a single regex

Comment 10

[Chirag Bhatia]

Attachment: 967196.patch

It turns out, tags are objects which have the actual tag strings in the name property, so I'm not sure if I can use the => operator within the .map() to return the proper tag object array. Let me know if I can and should do that.

Also, do you think we should include the tag string in the Deprecated.warning message to let the user know which tag was not trimmed?

I've also added a test for untagURI, and I used uri instead of makeURI (I'm guessing that was a typo at your end).

Let me know if I need to correct anything here.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Chirag Bhatia from comment #10)
> It turns out, tags are objects which have the actual tag strings in the name
> property, so I'm not sure if I can use the => operator within the .map() to
> return the proper tag object array. Let me know if I can and should do that.

"=>" is not an array/object operator, it just identified arrow functions, that are a more compact notation for functions with "this" bound at the creation time.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

function (a) {
  return a + 1;
}
should give you the same result as
a => a + 1

> Also, do you think we should include the tag string in the
> Deprecated.warning message to let the user know which tag was not trimmed?

I don't think it's worth it, it's a rare condition.

> I've also added a test for untagURI, and I used uri instead of makeURI (I'm
> guessing that was a typo at your end).

I used makeURI to allow you test that code in the browser console/scratchpad. makeURI is just a small browser util.
using uri is fine, using NetUtil.newURI is even better (uri is a custom function we created and could go away), all of them do the same thing, create a nsIURI and assign a spec to it.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8588061 [details] [diff] [review]
967196.patch

Review of attachment 8588061 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsTaggingService.js
@@ +9,5 @@
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> +Components.utils.import("resource://gre/modules/Deprecated.jsm");

Please use defineLazyModuleGetter for Deprecated.jsm, since it's used only in rare cases it's better to lazy load it.

@@ +142,5 @@
>  
> +    tags = tags.map(function(tag) {
> +      tag.name = tag.name.trim();
> +      return tag;
> +    });

As I said, you can compact this code with an arrow function

@@ +221,5 @@
>      // This also does some input validation.
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
> +    let isAnyTagNotTrimmed = tags.some(function(tag) {
> +      return (/^\s|\s$/.test(tag.name));

you can use an arrow function here too
tag => /^\s|\s$/.test(tag.name)

@@ +225,5 @@
> +      return (/^\s|\s$/.test(tag.name));
> +    });
> +
> +    if (isAnyTagNotTrimmed) {
> +      Deprecated.warning("At least one tag passed to untagURI was not trimmed", "https://bugzilla.mozilla.org/show_bug.cgi?id=967196");

please put the second argument on its own line, aligned with the first argument.

@@ +253,5 @@
>      if (!aTagName || aTagName.length == 0)
>        throw Cr.NS_ERROR_INVALID_ARG;
>  
> +    if (/^\s|\s$/.test(aTagName)) {
> +      Deprecated.warning("Tag passed to getURIsForTag was not trimmed", "https://bugzilla.mozilla.org/show_bug.cgi?id=967196");

same indentation

::: toolkit/components/places/tests/unit/test_tagging.js
@@ +181,2 @@
>    // cleanup
>    tagRoot.containerOpen = false;

There's no reason to add the test before the containerOpen = false call, they are unrelated.
Just move it later.

Also, you need to test something here, you are not testing anything.
after tagging you must use getURIsForTag to check the uri has been tagged with the trimmed value.
And then you can untag the trimmed value and check the tag has gone with untagURI

for those you can use Assert.equal, Assert.ok and similar helpers.

Comment 13

[Chirag Bhatia]

Attachment: 967196.patch

I've made a few changes according to what you've asked in the above comments.

I still have a few queries though:
I'm not able to figure out how to use the arrow function for trimming the tags in the tagURI method. Sorry if this is obvious, I'm new to arrow functions, I did read its documentation and a few examples on a few links but none of those helped in this scenario.

Also, I've done the indentation so that the characters do not exceed 80 characters in any line, let me know if I need to include the closing brackets with its preceding lines instead.

I've also made changes to the test file.

Let me know if anything in the patch needs to be fixed.

Comment 14

[Chirag Bhatia]

Attachment: 967196.patch

OK, figured out the arrow function. Didn't realize previously that I can have multiple statements within the arrow function by using blocks.

Comment 15

[Chirag Bhatia]

Attachment: 967196.patch

Minor change. Avoiding 'tagssvc' in favor of 'PlacesUtils.tagging' for consistency among the tests that I've added.

Comment 16

[Chirag Bhatia]

Attachment: 967196.patch

Ok, last change. Forgot to do indentation in the defineLazyModuleGetter line. Sorry for the multiple attachments and review flags.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8590995 [details] [diff] [review]
967196.patch

Review of attachment 8590995 [details] [diff] [review]:
-----------------------------------------------------------------

sorry for the ping pong game here, the code has too many unexpected legacy behaviors :/

::: toolkit/components/places/nsTaggingService.js
@@ +141,5 @@
>      // This also does some input validation.
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
> +    tags = tags.map(tag => {
> +      tag.name = tag.name.trim();

Ah, sigh :(
This could be a lazy getter and we don't want to trim at this point if it is, cause otherwise we'd "lock" down the tag name... Most of this is dumb legacy behavior that soon or later we'll remove, but for now we must bring it on.

At this point I'd say to add an optional "trim=false" argument to _convertInputMixedTagsArray, and pass true in tagURI. inside _convertInputMixedTagsArray, based on the value of the boolean trim argument, you should be able to set tag.name = trim? val.trim() : val;

@@ +223,5 @@
>      let tags = this._convertInputMixedTagsArray(aTags);
>  
> +    let isAnyTagNotTrimmed = tags.some(tag => /^\s|\s$/.test(tag.name));
> +
> +    if (isAnyTagNotTrimmed) {

you can remove the empty line from here

@@ +226,5 @@
> +
> +    if (isAnyTagNotTrimmed) {
> +      Deprecated.warning("At least one tag passed to untagURI was not trimmed",
> +                         "https://bugzilla.mozilla.org/show_bug.cgi?id=967196"
> +                        );

please just move the parenthesis on the previous line

@@ +256,5 @@
>  
> +    if (/^\s|\s$/.test(aTagName)) {
> +      Deprecated.warning("Tag passed to getURIsForTag was not trimmed",
> +                         "https://bugzilla.mozilla.org/show_bug.cgi?id=967196"
> +                        );

please just move the parenthesis on the previous line

Comment 18

[Chirag Bhatia]

Attachment: 961796.patch

Done :)

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8591706 [details] [diff] [review]
961796.patch

Review of attachment 8591706 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good.

We can already start using this:
Please remove the now useless trim() from here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#459
459           let tags = aData.tags.split(",").map(tag => tag.trim());

::: toolkit/components/places/nsTaggingService.js
@@ +10,5 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, 'Deprecated',
> +                                  'resource://gre/modules/Deprecated.jsm');

please use double apices " instead of single ones on both lines, it's a more common style

@@ +104,5 @@
>     *
>     * @throws Cr.NS_ERROR_INVALID_ARG if any element of the input array is not
>     *         a valid tag.
>     */
> +  _convertInputMixedTagsArray: function TS__convertInputMixedTagsArray(aTags, trim=false)

please update the javadoc above, add the
@param trim [optional]
       Whether to trim passed-in named tags.

::: toolkit/components/places/tests/unit/test_tagging.js
@@ +178,5 @@
> +
> +  // Tagging service should trim tags (Bug967196)
> +  let exampleURI = uri("http://www.example.com/");
> +  PlacesUtils.tagging.tagURI(exampleURI, [ " test " ]);
> +  

trailing spaces

@@ +181,5 @@
> +  PlacesUtils.tagging.tagURI(exampleURI, [ " test " ]);
> +  
> +  let exampleTags = PlacesUtils.tagging.getTagsForURI(exampleURI);
> +  do_check_eq(exampleTags.length, 1);
> +  do_check_eq(exampleTags[0], "test");

Just as a side note, now we don't use anymore the do_check_ functions, we user Assert.ok, Assert.equal and so on... It's not important here since this test has not been updated yet.

@@ +182,5 @@
> +  
> +  let exampleTags = PlacesUtils.tagging.getTagsForURI(exampleURI);
> +  do_check_eq(exampleTags.length, 1);
> +  do_check_eq(exampleTags[0], "test");
> +  

trailing spaces

Comment 20

[Chirag Bhatia]

Attachment: 961796.patch

Hi Marco,

I've made the changes that you asked for.

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8592913 [details] [diff] [review]
961796.patch

Review of attachment 8592913 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +458,1 @@
>            if (tags.length)

you removed too much code...
tags now is not defined anymore... we still need to do:
let tags = aData.tags.split(",");

::: toolkit/components/places/nsTaggingService.js
@@ +100,5 @@
>     *
>     * @param aTags
>     *        Array of tags.  Entries can be tag names or concrete item id.
> +   * @param trim [optional]
> +   *        Whether to trim passed-in named tags.

please add " Defaults to false."

Comment 22

[Chirag Bhatia]

Attachment: 961796.patch

Done :)

Comment 23

[Marco Bonardo [:mak]]

Attachment: unbitrotted patch

The batch bitrotted due to another patch landing first, I updated it to be able to push to try, unfortunately try is currently closed, I'll retry later.

Comment 24

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ea1c540f7d

Comment 25

[Marco Bonardo [:mak]]

ugh the commit message is wrong... let me fix it.

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c8d12213943f

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8d12213943f