Closed Bug 1266469 Opened 8 years ago Closed 7 years ago

PRIMARY_KEY typo in PlacesDBUtils

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox48 --- affected

People

(Reporter: mak, Assigned: mak, Mentored)

References

Details

There's a PRIMARY_KEY (should be PRIMARY KEY) typo in PlacesDBUtils that makes so that the table is not built properly.
This may break database cleanups for some users.

Also, that seems to indicate test D.10 is broken?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_preventive_maintenance.js#653
The scope here is to fix the typo, and then understand why the test here:
https://dxr.mozilla.org/mozilla-central/rev/b1b9129838ade91684574f42219b2010928d7db4/toolkit/components/places/tests/unit/test_preventive_maintenance.js#668
didn't detect this problem.
Mentor: mak77
Whiteboard: [good first bug][lang=js]
hi, I think I could work on this. thanks.
(In reply to Carol Chung[:lizardwalk5] from comment #2)
> hi, I think I could work on this. thanks.

Awesome! Please see https://wiki.mozilla.org/Contribute for how to get started.
thanks! I started looking at the docs and specified code. I believe that for this summer, I am really looking for a long term project so I am going to release this bug for someone else to work on (or if it's free around Sept, I will check back).

however, I can contribute the links to the docs I felt helpful, point to a couple broken links, and ask a couple questions that may be useful to the next bug fixer.

Useful docs:
  https://wiki.mozilla.org/Places
  https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places
  https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Database
    see Bookmarks Tables > moz_bookmarks
  https://wiki.mozilla.org/Places:Design_Overview
  https://wiki.mozilla.org/images/d/d5/Places.sqlite.schema3.pdf
  http://kb.mozillazine.org/Places.sqlite

Broken links (docs):
  http://people.mozilla.org/~dietrich/places-erd.png
  (for the above link, I assume https://wiki.mozilla.org/images/d/d5/Places.sqlite.schema3.pdf would be the equivalent doc but not sure if it is up to date?)
  https://developer.mozilla.org/en/Places/Bookmarks_Service/Design

Pointer to the code fix:
  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#558
  'PRIMARY_KEY' => 'PRIMARY KEY'

Questions:
  * What is the scenario in which a folder(s) would end up with a bad position value(s)? What would cause that? How would you replicate that in a sqlite db?
  * I'm having a challenge with understanding the logic behind the triangular numbers equation. Should I need to look up some documentation about how/why it works? referenced: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#551
    also referenced: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#572
  * I'm having a challenge with understanding the pattern of how the moz_bookmarks table > parent values get assigned. for the default folders, should I assume the parent values are just arbitrary?
  * Do separators determine the folder's parent value?
  * One point of confusion is that I can kind of see the logic to the Other Bookmarks folder (parent: 1, position: 3) values. However when I created a child folder under Other Bookmarks, I don't understand the resulting parent value (parent: 5, position: 0). I expected the new folder's parent value to be closer to 1 (like 2 perhaps?).
  * Does it seem like these kinds of questions are relevant to troubleshooting the associated test? thanks.
(In reply to Carol Chung[:lizardwalk5] from comment #4)

sorry, but if some of these questions should be plainly answered by my looking more closely at more of the code, then please let me know. I should not be lazy.

also another question: should there be a way to run the test in question in isolation? and be able to stop it in the middle? or is the expectation to debug the test mainly by just looking at the code?

> Questions:
>   * What is the scenario in which a folder(s) would end up with a bad
> position value(s)? What would cause that? How would you replicate that in a
> sqlite db?
>   * I'm having a challenge with understanding the logic behind the
> triangular numbers equation. Should I need to look up some documentation
> about how/why it works? referenced:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesDBUtils.jsm#551
>     also referenced:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesDBUtils.jsm#572
>   * I'm having a challenge with understanding the pattern of how the
> moz_bookmarks table > parent values get assigned. for the default folders,
> should I assume the parent values are just arbitrary?
>   * Do separators determine the folder's parent value?
>   * One point of confusion is that I can kind of see the logic to the Other
> Bookmarks folder (parent: 1, position: 3) values. However when I created a
> child folder under Other Bookmarks, I don't understand the resulting parent
> value (parent: 5, position: 0). I expected the new folder's parent value to
> be closer to 1 (like 2 perhaps?).
>   * Does it seem like these kinds of questions are relevant to
> troubleshooting the associated test? thanks.
(In reply to Carol Chung[:lizardwalk5] from comment #4)
> Useful docs:
>   https://wiki.mozilla.org/Places
>   https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places
>   https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Database
>     see Bookmarks Tables > moz_bookmarks
>   https://wiki.mozilla.org/Places:Design_Overview
>   https://wiki.mozilla.org/images/d/d5/Places.sqlite.schema3.pdf
>   http://kb.mozillazine.org/Places.sqlite

Some of these are likely outdated, we've bee na bit lazy on updating docs since the lack of resources.
> Pointer to the code fix:

> Questions:
>   * What is the scenario in which a folder(s) would end up with a bad
> position value(s)? What would cause that? How would you replicate that in a
> sqlite db?

There is no specific scenario, it could be an add-on running the wrong query on the database, or an internal bug.
You can replicate the problem running a raw SQL query on places.sqlite that causes the problem.

>   * I'm having a challenge with understanding the logic behind the
> triangular numbers equation. Should I need to look up some documentation
> about how/why it works? referenced:

it's pretty simple, every folder has a number of children, every child has an index. we sum up the index value for each children (increased by one), then we compare the result with (n * (n + 1) / 2) where n is the number of children.
The theory is explained here: https://en.wikipedia.org/wiki/Triangular_number

>   * I'm having a challenge with understanding the pattern of how the
> moz_bookmarks table > parent values get assigned. for the default folders,
> should I assume the parent values are just arbitrary?

It's a hierarchy, every row in the db has an id and a parent, the parent is the id of the containing folder
id | parent | index | 
 1 |      0 |     0 | folder
 2 |      1 |     0 | bookmark inside folder
 3 |      1 |     1 | separator inside folder

>   * Do separators determine the folder's parent value?

no, separators are just like bookmarks, they can be children of a folder

>   * One point of confusion is that I can kind of see the logic to the Other
> Bookmarks folder (parent: 1, position: 3) values. However when I created a
> child folder under Other Bookmarks, I don't understand the resulting parent
> value (parent: 5, position: 0). I expected the new folder's parent value to
> be closer to 1 (like 2 perhaps?).

if you add a folder inside other bookmarks, its parent will be the other bookmarks id

>   * Does it seem like these kinds of questions are relevant to
> troubleshooting the associated test? thanks.

It may help understanding why the test is not failing :)
(In reply to Carol Chung[:lizardwalk5] from comment #5)
> also another question: should there be a way to run the test in question in
> isolation? and be able to stop it in the middle? or is the expectation to
> debug the test mainly by just looking at the code?

You can use ./mach xpcshell-test src_relative_path_to_test to execute the whole test file, you can comment out not relevant part to run the specific sub test, if necessary.
I think you can use --jsdebugger option to attach the jsdebugger (but that requires adding firefox-appdir = browser to xpcshell.ini head section) and use "debugger;" where you want to breakpoint.
Or just use plain dump("message\n") to follow the code.
Priority: P1 → P2
hi Marco, thanks for your responses. I think I have bandwidth to work on this project. (this is the same person as :lizardwalk5 but I have this other mozilla account for a summer internship) thanks.
(In reply to Carol Chung from comment #8)
> hi Marco, thanks for your responses. I think I have bandwidth to work on
> this project. (this is the same person as :lizardwalk5 but I have this other
> mozilla account for a summer internship) thanks.

I wanted to follow up that I'll give myself a 2 week deadline to work on this and if I cannot progress then release the issue afterward.

I've been thinking about the test and at a very high level was thinking maybe the randomization logic wasn't enough to fail the triangulation equation check (because while the parent assignment would be jumbled, it seems like the total sum would remain the same). but I still need to think about your responses, run tests and think of a better test logic (maybe perhaps manually assign parent value that is out of the expected range or manually remove a parent value, although I'm not sure if either is a good simulation of a likely real world event).
Hi, there is no update on whether this bug is fixed or not.  I am a student in open source and looking for my first bug, if its available, Can I work on it? thanks
no, this bug is not fixed and it still requires someone to investigate and work on it.
Thanks,any tips before I start it.
be sure you have a working build environment and that you can run tests using "./mach test path_to_test"
Most of the info is at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
You can needinfo me here for questions, or ping me in the #fx-team IRC channel (if I don't reply I'm not around)
Most info about this specific bug are spread in the previous comments.
Hi! I am starting with Open source and attempting to solve this as my first bug.
Hi 

I will work on it and have it fixed
Just wanted to make sure I'm not doubling somebody's else's job
It is my bug? :)

Do you agree? :)

Regards
I think not having any signal of progress in a couple weeks is enough to consider the bug stale.
I'll fix this in a more general cleanup bug I'm working on... The reason this has no effect is that Sqlite ignores the invalid PRIMARY_KEY attribute. The difference is that querying by id won't use an index and will be slower.
On the other side, defining id as a primary key would make it an alias of rowid, that would break the query itself because rowid wouln't grow monotoniacally anymore.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js]
Depends on: 1422058
Fixed by bug 1422058
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.