okfn-brasil / cuidando2

Cuidando do Meu Bairro 2.0 é uma reescrita do projeto Cuidando do Meu Bairro ( cuidando.org.br ) que busca mapear a execução do orçamento municipal de São Paulo.
GNU Affero General Public License v3.0
6 stars 2 forks source link

API retornando 404 para uma exceção NoResultFound #80

Closed alexandre closed 9 years ago

alexandre commented 9 years ago

Nesse caso não deveria ser um 204? (No Content) Afinal, a rota (resource) existe, apenas aquela chave/filtro não está relacionada com qualquer informação.

[ ]'s

andresmrm commented 9 years ago

Hummmmm... Faz sentido. Tem que mudar isso em vários lugares...

vatsu commented 9 years ago

Acho bem legal padronizar e gosto de 204... Só precisa tomar cuidado para não mandar body como aqui:

def get_comment(comment_id):
    try:
        return db.session.query(Comment).filter_by(id=comment_id).one()
    except NoResultFound:
        api.abort(404, 'Comment not found')

Aliás precisa ver se com coleções de resources e um único resource vai usar 204..Hoje tem uso de 200 parar coleções:

def get_thread_comments(thread_name):
    try:
        thread = (db.session.query(Thread)
                    .filter(Thread.name == thread_name).one())
    except NoResultFound:
        return {'comments': []}
        # api.abort(404)
    return {
        'comments': [
            {
                'id': c.id,
                'text': c.text,
                'author': c.author.name,
                'created': str(c.created),
                'modified': str(c.modified),
                'likes': c.likes,
                'dislikes': c.dislikes,
            }
            for c in thread.comments
        ]
    }
andresmrm commented 9 years ago

Oi, @vatsu !

Não sei se entendi bem o que você quis dizer com "Só precisa tomar cuidado para não mandar body como aqui". Você está dizendo essa linha api.abort(404, 'Comment not found')? Ela não retorna a mensagem direto não. Mas sim um Json como esse: { "message": "Comment not found", "status": 404 }. Era essa a sua dúvida?

A segunda parte também não entendi... =P Ele está retornando uma lista vazia quando não acha a thread, com código 200 mesmo.

alexandre commented 9 years ago

@vatsu ,

"Só precisa tomar cuidado para não mandar body como aqui". Não faz sentido, se o status code é 204, você simplesmente ignora o body; na verdade (ou AFAIK), o flask-restful, por exemplo, nem manda. =]

@andresmrm ,

Há algum tempo eu fiz essa brincadeira aqui: https://github.com/alexandre/flask-rest-template/blob/master/app/helpers.py#L75

[ ]'s

LuizArmesto commented 9 years ago

NoResultFound deve retornar um 404 mesmo. Se o cliente fez uma requisição para um resource que não existe a resposta do servidor deve ser de falha (familia 400, no caso de não encontrado, 404). A família 200 é de requisição bem sucedida, o que não é o caso. Se você pediu um resource que não foi encontrado a sua requisição não foi bem sucedida.

alexandre commented 9 years ago

@LuizArmesto ,

Dá uma olhada aqui: https://github.com/okfn-brasil/esiclivre/blob/master/esiclivre/views.py#L129

O usuário fez um request para um resource que existe, o quê não existe são dados para o parametro passado...IMO, é motivo o suficiente para um 204 ao invés de um 404. Ainda usaria alguém da família 4xx se o payload estiver inválido (e.g. bad request com 400)

LuizArmesto commented 9 years ago

Resource é o conteúdo. Se ele fez uma requisição para "/pedidos/id/42" e não tem nenhum pedido com o id 42, então o resource que ele pediu não existe e o erro que deve ser retornado é 404.

vatsu commented 9 years ago

IMHO, Se é rota tipo GET /pedidos/id/42 204 é estranho. Melhor seria 404 ou 410 (se o resource tiver sido deletado e houver soft delete para checar isso). Mas em rotas do tipo GET /pedidos 204 pode faz sentido...

2015-08-30 12:23 GMT-03:00 Luiz Armesto notifications@github.com:

Resource é o conteúdo. Se ele fez uma requisição para "/pedidos/id/42" e não tem nenhum pedido com o id 42, então o resource que ele pediu não existe e o erro que deve ser retornado é 404.

— Reply to this email directly or view it on GitHub https://github.com/okfn-brasil/cuidando2/issues/80#issuecomment-136153660 .

LuizArmesto commented 9 years ago

É o que o @vatsu disse. Em rotas como "/pedidos" não faz sentido o 404 mesmo, pois é uma collection, e a collection existe independente de estar vazia ou não.

No caso do "/pedidos/id/42" o 204 faria sentido num POST, para criar um pedido com o id 42, mas o melhor é fazer o POST na collection e deixar o sistema associar um id calculado internamente. Também sou inclinado a chamadas POST retornarem sempre o resource salvo e não 204.

alexandre commented 9 years ago

Entendi os dois pontos; que fique como está então.

@andresmrm se você quiser, pode fechar a issue. =]

andresmrm commented 9 years ago

Ok! =)