Closed Bug 1152915 Opened 9 years ago Closed 9 years ago

Readinglist server rejecting POSTs with blank titles

Categories

(Cloud Services :: Server: ReadingList, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bobm, Assigned: tarek)

Details

(Whiteboard: [readinglist-1.7])

Attachments

(1 file)

56 bytes, text/x-github-pull-request
rhubscher
: review+
Details | Review
The Readinglist server is rejecting POSTs with blank titles as malformed requests, but blank titles are valid.  

Example request JSON:
{"is_article":false,"added_on":1424983452123,"resolved_title":null,"added_by":"$local","read_position":0,"url":"http:\/\/en.wikipedia.org\/wiki\/Library_cat","title":"","word_count":0,"excerpt":null,"favorite":false,"resolved_url":null,"stored_on":null,"_id":2,"archived":false,"unread":true}

Example server response:

    HTTP/1.1 400 Bad Request
    Date: Thu, 09 Apr 2015 18:02:02 GMT
    Content-Type: application/json; charset=UTF-8
    Content-Length: 89
    Connection: keep-alive
    Access-Control-Expose-Headers: Backoff, Retry-After, Alert
     
    {"errno":107,"message":"title in body: Required","code":400,"error":"Invalid parameters"}
Since when blank titles are valid?
Flags: needinfo?(rnewman)
I think it is a client error.

resolved_title can be empty but the user title must not be blank.
This is a future to guarantee clients they will have a title to display an article.
Added other clients as needinfo. Let's make sure everyone is on the same page for this.

Data model fields are detailed here: 

* http://readinglist.readthedocs.org/en/latest/model.html#articles
* http://readinglist.readthedocs.org/en/latest/api/resource.html#post-articles (with which ones are optional)
Flags: needinfo?(sarentz)
Flags: needinfo?(mhammond)
Seems strange to allow a 1 character title - I'd have thought no title is better than an obviously invalid one. I guess I don't see the point of this TBH - why not let the client decide what to do if no title?  If the client fails to get a valid title for any reason and wants to add the URL anyway, it's just going to set the title to the url - otherwise the locally existing item will never sync, which seems a poor outcome.

IOW, for "required" items in general, if we got a report of us failing to get a value thus causing the sync to fail, we'd just arrange to invent a hopefully-sane value - what else would we do?  Allowing null seems a better outcome - it's a good clue that a (different?) client should get one if it can.
Flags: needinfo?(mhammond)
But Drew's really the desktop guy who matters here ;)
Flags: needinfo?(adw)
Indeed, currently, we strip the spaces at beginning/end of the title, and require it to have (once stripped) a length of 1 minimum.

We need to have a confirmation whether:

* the title field is optional on creation, and if so, what is its default value;
* the title field can be empty if provided;
* the title should be validated with some additionnal regexp (beyond length and blanks).

This is rather trivial to change on our side, so once we have instructions, we'll be able to release a fix immediately!

Thanks everyone for your feedback!
I apologize if any of my previous answers sounds curt, I didn't mean them to be.
My vote would be to allow null/empty on creation and silently ignore attempts to update to null/empty later. But I think desktop always supplies this currently.
We could also default an empty title to the url basename.
(In reply to Rémy Hubscher (:natim) from comment #9)
> We could also default an empty title to the url basename.

As I mentioned in comment 4, I think null is a good signal a client can update it - eg, see bug 1141476.
There is real content on the Web with empty titles. Forcing the client to come up with something seems incorrect. Blank titles are valid; let the client fall back to whatever is suitable when displaying, rather than polluting the data. 

(in particular, we very often won't know the title when we first add an item. That's part of the title/resolved_title dichotomy)
Flags: needinfo?(rnewman)
Attached file PR 250
Attachment #8591947 - Flags: review?(rhubscher)
(Replying from a real keyboard)

btw: the reason I originally suggested that title be non-optional was to ensure that a client didn't forget to include it, and to encourage them to do so, not to require that it be non-*empty*.

Every item we add from Twitter or other Android apps will have an empty title, and we'll later fill in resolved_title.
Flags: needinfo?(adw)
https://github.com/mozilla-services/readinglist/commit/8a167ebbaacb12c329a884673fa25c9779121a6c
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(sarentz)
Resolution: --- → FIXED
Attachment #8591947 - Flags: review?(rhubscher) → review+
Whiteboard: [readinglist-1.7]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: