Ori Liel has posted comments on this change.
Change subject: restapi: Mappers for Gluster entities
......................................................................
Patch Set 2: (12 inline comments)
Some comments inside and I still need to get 100% approval for the modelling
(I'm still consulting with PMs).
All in all looks good; the comments are mostly technical comments about
conventions in the rest-api project.
Contact me for anywhing
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterBrickMapper.java
Line 8:
In our application only business entities have Mapper classes (a convention).
Since gluster-brick is not a business entity, but rather is part of the
Gluster-Volume entity, can you please move these map() functions into
GlusterVolumeMapper?
Line 22:
brick.setStatus(GlusterBrickStatus.valueOf(fromBrick.getStatus().name()));
1) for all fields, add isSet() before referencing them, to defend against
NullPointerException. For example, when setting the server-ID, if no server-ID
exists, Guid.createGuidFromString would fail with NPE. If a field is always
expected to be set, we validate against this in the appropriate places, but the
mapper should be oblivious to this and know how to handle null. Also, I know
that a Brick is immutable, but as a principal, the mapper is used both for
creating new entites and modifying them. When modifying (updating) and entity,
we map the fields ON TOP of an existing entity. So if you receive null for some
field, because the user didn't set it, and you don't check isSet(), then even
if there is no NPE, you are at risk of overriding a valid value with null, and
sending this update to the Backend.
For usage of isSet() Take example from any other mapper.
2) for enum values the mapping will need to change because of the removal of
enums from api.xsd (see my comments in the other patch)
3) specifically for status field - this mapping should be only one way: from
the Backend-->REST-API. The user can't update the status, so there is no point
in mapping it. The server will ignore it anyway (or even fail for it), and it's
just confusing to the eye.
Line 31:
brick.setStatus(GlusterStatus.valueOf(fromBrick.getStatus().name()));
1) Same comment about danger for NullPointerException, except in this direction
you won't have isSet() method (those are generated by JaxB and are only
available on the REST-API entities) so simply check (==null)
2) same comment for enum values
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterOptionMapper.java
Line 6:
In our application only business entities have Mapper classes (a convention).
Since gluster-volume is not a business entity, but rather is part of the
Gluster-Volume entity, can you please move these map() functions into
GlusterVolumeMapper?
Line 13: return option;
Please add null validations, see full comment in GlusterBrickMapper
Line 21: return option;
Please add null validations, see full comment in GlusterBrickMapper
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterVolumeMapper.java
Line 54: }
mapping for status field should only be from Backend-->REST-API, see
GlusterBrickMapper for full comment
Line 58:
same comment about null validations and enums, see GlusterBrickMapper
Line 90: return volume;
same comment about null validations and enums, see GlusterBrickMapper
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/GlusterBrickMapperTest.java
Line 23: assertEquals(model.getStatus(), transform.getStatus());
When you will delete the mapping of status in the direction REST-API-->Backend
(see mapper files for this comment), you will see that the test fails, because
status doesn't do a 'round trip' any more. You can see an example for
non-round-trip mapping test in: DataCenterMapperTest
Also, if the class GlusterBrickMapper is removed and it's functionality moved
into GlusterVolumeMapper (see GlusterBrickMapper for this comment) then this
testing functionality should move accordingly to GlusterBrickMapperTest
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/GlusterVolumeMapperTest.java
Line 19: model.setAccessProtocols("GLUSTER,NFS");
Well spotted, this is confusing :)
Line 82: }
When you will delete the mapping of status in the direction REST-API-->Backend
(see mapper files for this comment), you will see that the test fails, because
status doesn't do a 'round trip' any more. You can see an example for
non-round-trip mapping test in: DataCenterMapperTest
--
To view, visit http://gerrit.ovirt.org/3385
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I364d853106eda1a956241eb7210ce85781ee88b9
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
