Closed Bug 474334 Opened 12 years ago Closed 12 years ago

"place:" is a valid query to deserialize and should not cause an "ignoring unknown key:" warning


(Toolkit :: Places, defect)

Not set





(Reporter: mak, Assigned: mak)


(Keywords: fixed1.9.1)


(1 file)

Attached patch patch v1.0Splinter Review
Seen while looking at bug 370197.

When we have a query where we only defined default values (for example expandQueries = 1 and queryType=QUERY_TYPE_HISTORY) and we serialize it, we end up with a query like "place:".
When we deserialize this query we get a WARNING since we are trying to handle "place:" as a query option... instead we should simply return an array with a new default query in it, since that's a valid case.

i'll add a test for this.
Isn't the test for this covered by adw's work in bug 370197?
could be, but it is not atm, i still have to look at the reason it's not failing, so if that can be fixed there i'll not create a new test, clearly.
mh instead this is currently working, even if the code is wrong, because when we get tokens we wrongly get a wrong token like "place:". Having at least one token we still try to deserialize, but we warn at
because "place:" is not a valid option.

So, the final result is the same, we get a valid default query, but through a wrong path. The test cannot fail due to that, and is indeed correctly testing this.

Moreover i cannot write a test to check an internal method when API is still returning a correct value.
Summary: "place:" shoud be a valid query to deserialize → "place:" is a valid query to deserialize and should not cause an "ignoring unknown key:" warning
Attachment #357712 - Flags: review?(dietrich)
Comment on attachment 357712 [details] [diff] [review]
patch v1.0

test in bug 370197 is valid to cover deserialization of "place:", the wrong path can't be tested though.
Attachment #357712 - Flags: review?(dietrich) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 357712 [details] [diff] [review]
patch v1.0

asking approval, low risk
Attachment #357712 - Flags: approval1.9.1?
Target Milestone: --- → mozilla1.9.2a1
Attachment #357712 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.