Martin Mucha has posted comments on this change. Change subject: restapi: return 404 when there are inexisting resources on path ......................................................................
Patch Set 26: (2 comments) https://gerrit.ovirt.org/#/c/39499/26/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResourceTest.java: Line 10: @Override Line 11: protected void verifyIfHostExistsToHandle404StatusCode() { Line 12: //very disgusting hack. Test dictates, that 400 is returned in case of wrong parameters. But 404 Line 13: // has precedence to that, so production code should be able to return 404 when necessary, but I Line 14: // have no idea how to convince rests test to be ok with that. Any help welcomed. > The reason for the failure of this test is not that the test dictates 400, thanks for example patch; with it I was able to fix those test. However those tests still tests only 400 status code, tests for 404 are missing. But as I understand, there might be discussion whether even merge this, so I'll add these only if needed not to waste time on that. You're right. I know about negative impact, but I think we talked about it you were ok with it, I surely talked about it with moti and he was also ok about it. Now. Performance impact is present, although not that big. And this action won't be run that often (actually probably not often at all), so it's marginal (although not clean). But I'd be more concerned about validity toward rest clients. When referencing inexisting resource 404 or at least some error should be returned. Definitely not 200. Line 15: } Line 16: }, VdcQueryType.GetNetworkAttachmentsByHostId); Line 17: } https://gerrit.ovirt.org/#/c/39499/26/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostNicNetworkAttachmentsResourceTest.java File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendHostNicNetworkAttachmentsResourceTest.java: Line 14: @Override Line 15: protected void verifyHostAndNicExistence() { Line 16: //very disgusting hack. Test dictates, that 400 is returned in case of wrong parameters. But 404 Line 17: // has precedence to that, so production code should be able to return 404 when necessary, but I Line 18: // have no idea how to convince rests test to be ok with that. Any help welcomed. > See the comments here (in this same patch): Done Line 19: } Line 20: Line 21: protected void verifyIfHostExistsToHandle404StatusCode() { Line 22: //very disgusting hack. Test dictates, that 400 is returned in case of wrong parameters. But 404 -- To view, visit https://gerrit.ovirt.org/39499 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3e96ebfee8bbf2c6c1714d9d652fa5cc5e46886 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
