Juan Hernandez has posted comments on this change.
Change subject: restapi: Return Additional Info For 400 Messages (#867794)
......................................................................
Patch Set 1:
(11 comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 117: </xs:sequence>
Line 118: </xs:complexType>
Line 119:
Line 120:
Line 121: <xs:element name="usage_message" type="UsageMessage"/>
Add whitespace before this, like in the other element definitions.
Line 122:
Line 123: <xs:complexType name="UsageMessage">
Line 124: <xs:sequence>
Line 125: <xs:element name="message" type="xs:string"/>
Line 125: <xs:element name="message" type="xs:string"/>
Line 126: <xs:element ref="detailedLink" minOccurs="0" maxOccurs="1"/>
Line 127: </xs:sequence>
Line 128: </xs:complexType>
Line 129:
Remove this whitespace.
Line 130: <!-- Actions -->
Line 131:
Line 132: <xs:complexType name="GracePeriod">
Line 133: <xs:sequence>
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/validation/UsageFinder.java
Line 14: import org.ovirt.engine.api.restapi.resource.BackendApiResource;
Line 15:
Line 16: public class UsageFinder {
Line 17:
Line 18: private static final String RESPONSE = "Request Syntactically
Incorrect. Correct Usage:";
Please don't use upper case in all the words, only the first:
Request syntactically incorrect. Correct usage:
If I understand correctly what we will be doing is returning a document like
this:
<usage_message>
<message>Request syntactically incorrect. Correct Usage:</message>
<link href="/api/vms/{vm:id}" rel="update">
...
</link>
</usage_message>
The content of this <link> will be the same that we include in the RSDL. Can
you confirm this? If this is correct I would suggest a different wording of the
message:
Request syntactically incorrect. See the link description below for the
correct usage.
Also I would appreciate if you can include an example of the returned document
in the commit message.
Line 19: private static final String UUID_REGEX =
Line 20:
"^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}";
Line 21:
Line 22: public UsageMessage getUsageMessage(Application application,
UriInfo uriInfo, Request request) {
Line 31: if (isMatch(link, uriInfo, httpMethod)) {
Line 32: return link;
Line 33: }
Line 34: }
Line 35: return null; // should never happen
For this kind of things that should never happen I think you should either
throw an exception or at least generate a error message in the log, otherwise
it is very difficult to diagnose when they actually happen.
Line 36: }
Line 37:
Line 38: private RSDL getRSDL(Application application) {
Line 39: for (Object obj : application.getSingletons()) {
Line 34: }
Line 35: return null; // should never happen
Line 36: }
Line 37:
Line 38: private RSDL getRSDL(Application application) {
As the RSDL is useful in many context I think that it deserves a class to
manage it. Would do you agree to add a RSDLManager class? Something like this:
public class BackendRSDLManager {
private RSDL rsdl = ...;
public static RSDL getRSDL() {
return rsdl;
}
}
We can then put in this class all the specifics of building the RSDL for the
backend, and avoid the loop below when looking it up.
We could also reuse this class in other places where we need the RSDL.
Line 39: for (Object obj : application.getSingletons()) {
Line 40: if (obj instanceof BackendApiResource) {
Line 41: BackendApiResource resource = (BackendApiResource) obj;
Line 42: return resource.getRSDL();
Line 41: BackendApiResource resource = (BackendApiResource) obj;
Line 42: return resource.getRSDL();
Line 43: }
Line 44: }
Line 45: return null; // should never happen
Same as before.
Line 46: }
Line 47:
Line 48: private boolean isMatch(DetailedLink link, UriInfo uriInfo, String
httpMethod) {
Line 49: int baseUriLength = uriInfo.getBaseUri().getPath().length();
Line 59: private boolean isMatchLength(String[] linkPathSegments,
List<PathSegment> uriPathSegments) {
Line 60: return linkPathSegments.length == uriPathSegments.size();
Line 61: }
Line 62:
Line 63: private boolean isMatchePath(String[] linkPathSegments,
List<PathSegment> uriPathSegments) {
s/isMatche/isMatch/
Line 64: for (int i = 0; i < linkPathSegments.length; i++) {
Line 65: String uriPathSegment = uriPathSegments.get(i).getPath();
Line 66: if (isUUID(uriPathSegment) ||
uriPathSegment.equals(linkPathSegments[i])) {
Line 67: continue;
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/rsdl/RsdlBuilder.java
Line 90:
Line 91: private static final String RESOURCES_PACKAGE =
"org.ovirt.engine.api.resource";
Line 92: private static final String PARAMS_METADATA = "rsdl_metadata.yaml";
Line 93:
Line 94: public RsdlBuilder(UriInfo uriInfo, ApplicationMode
applicationMode) {
If we create a class to manage the RSDL, then the builder shouldn't receive
applicationMode as parameter, but the list of "rels".
Line 95: this.uriInfo = uriInfo;
Line 96: this.applicationMode = applicationMode;
Line 97: this.parametersMetaData = loadParametersMetaData();
Line 98: }
Line 246: List<DetailedLink> results = new ArrayList<DetailedLink>();
Line 247: List<Class<?>> classes =
ReflectionHelper.getClasses(RESOURCES_PACKAGE);
Line 248: List<String> paths =
Line 249: applicationMode == ApplicationMode.GlusterOnly ?
ApiUtils.getGlusterRels(uriInfo)
Line 250: : ApiUtils.getAllRels(uriInfo);
If we create a class to manage the RSDL then this logic to decide which "rels"
to use should be there and not here.
Line 251: for (String path : paths) {
Line 252: Class<?> resource = findResource(path, classes);
Line 253: String entryPointPath = uriInfo.getBaseUri().getPath();
Line 254: String prefix = entryPointPath.endsWith("/") ?
entryPointPath + path : entryPointPath + "/" + path;
Line 249: applicationMode == ApplicationMode.GlusterOnly ?
ApiUtils.getGlusterRels(uriInfo)
Line 250: : ApiUtils.getAllRels(uriInfo);
Line 251: for (String path : paths) {
Line 252: Class<?> resource = findResource(path, classes);
Line 253: String entryPointPath = uriInfo.getBaseUri().getPath();
This computation of entryPointPath should be outside the for loop.
Line 254: String prefix = entryPointPath.endsWith("/") ?
entryPointPath + path : entryPointPath + "/" + path;
Line 255: results.addAll(describe(resource, prefix, new
HashMap<String, Type>()));
Line 256: }
Line 257: return results;
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/ApiUtils.java
Line 13: import org.ovirt.engine.api.model.Link;
Line 14: import org.ovirt.engine.api.model.Parameter;
Line 15: import org.ovirt.engine.api.model.ParametersSet;
Line 16:
Line 17: public class ApiUtils {
This is a great idea, but I think the name shouldn't be ApiUtils, but something
like BackendRSDLManager, as what it contains is basically the information/logic
to populate the RSDL for the backend.
Line 18:
Line 19: public static Collection<DetailedLink> getLinks(UriInfo uriInfo) {
Line 20: Collection<DetailedLink> links = new
LinkedList<DetailedLink>();
Line 21: links.add(createLink("capabilities", uriInfo));
--
To view, visit http://gerrit.ovirt.org/23017
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7a6da4fbb00ea5f1e6919ddfe21dd7b3dd126fd
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ori Liel <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
