Skip to content
This repository was archived by the owner on Feb 8, 2019. It is now read-only.

modified JaxpRequest and JaxpResponse's load method #7

Open
duanshiqiang wants to merge 2 commits into
base: master
Choose a base branch
from
Open

modified JaxpRequest and JaxpResponse's load method #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d3b9e1
modified JaxpRequest and JaxpResponse's load method
duanshiqiang Oct 20, 2016
78b81d3
removed init method to create the jaxb context object
duanshiqiang Oct 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions openaz-xacml/src/main/java/org/apache/openaz/xacml/std/jaxp/JaxpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.apache.openaz.xacml.std.dom.DOMUtil;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

/**
Expand All @@ -60,7 +59,9 @@
*/
public class JaxpRequest extends StdMutableRequest {
private static Log logger = LogFactory.getLog(JaxpRequest.class);


private static JAXBContext context = null;

public JaxpRequest() {
}

Expand Down Expand Up @@ -114,19 +115,17 @@ public static JaxpRequest load(File fileXmlRequest) throws ParserConfigurationEx
logger.error("No Document returned parsing \"" + fileXmlRequest.getAbsolutePath() + "\"");
return null;
}

NodeList nodeListRoot = document.getChildNodes();
if (nodeListRoot == null || nodeListRoot.getLength() == 0) {
logger.warn("No child elements of the XML document");
return null;
}
Node nodeRoot = nodeListRoot.item(0);
if (nodeRoot == null || nodeRoot.getNodeType() != Node.ELEMENT_NODE) {
logger.warn("Root of the document is not an ELEMENT");
return null;

Node nodeRoot = document.getDocumentElement();

if(context == null) {
synchronized (JaxpRequest.class) {
Copy link

@jainh jainh Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you are breaking DRY principle, again -1 for that. Thing, again to make it better, separate class is always a good option.

Copy link
Author

@duanshiqiang duanshiqiang Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come? The two context in JaxpRequest nad JaxpResponse are different.

if(context == null) {
context = JAXBContext.newInstance(RequestType.class);
}
}
}

JAXBContext context = JAXBContext.newInstance(RequestType.class);

Unmarshaller unmarshaller = context.createUnmarshaller();
JAXBElement<RequestType> jaxbElementRequest = unmarshaller.unmarshal(nodeRoot,
RequestType.class);
Expand Down
25 changes: 12 additions & 13 deletions openaz-xacml/src/main/java/org/apache/openaz/xacml/std/jaxp/JaxpResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.apache.openaz.xacml.std.dom.DOMUtil;
import org.w3c.dom.Document;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;

/**
Expand All @@ -59,7 +58,9 @@
*/
public class JaxpResponse extends StdMutableResponse {
private static Log logger = LogFactory.getLog(JaxpResponse.class);


private static JAXBContext context = null;

protected JaxpResponse() {
Copy link

@jainh jainh Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is protected ?

}

Expand Down Expand Up @@ -102,18 +103,16 @@ public static JaxpResponse load(File fileXmlResponse) throws ParserConfiguration
return null;
}

NodeList nodeListRoot = document.getChildNodes();
if (nodeListRoot == null || nodeListRoot.getLength() == 0) {
logger.warn("No child elements of the XML document");
return null;
}
Node nodeRoot = nodeListRoot.item(0);
if (nodeRoot == null || nodeRoot.getNodeType() != Node.ELEMENT_NODE) {
logger.warn("Root of the document is not an ELEMENT");
return null;
}
Node nodeRoot = document.getDocumentElement();

JAXBContext context = JAXBContext.newInstance(ResponseType.class);
if(context == null) {
Copy link

@jainh jainh Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need all this just do this

static {
        try {
            // one time instance creation
            jaxbContext = JAXBContext.newInstance(Request.class, Response.class);
        } catch (JAXBException e) {
            e.printStackTrace();
        }
    }

for both request and respone in a singleton class and reuse as required.

Copy link
Author

@duanshiqiang duanshiqiang Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this, jaxbContext will be initialized at application boot time no matter user need to use load method or not. Put it into the load method essentially lazy load it to save some memory.

Copy link

@jainh jainh Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok, it is a trade off better than having two places with synchronized block. You need it anyhow definitely.

Copy link
Author

@duanshiqiang duanshiqiang Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel it's good, we use the JaxpResponse to load XACML response string to an Response object and we construct the ResponseType object by our own unmarshal code. So it make sense to use the class without using it's own load method.

Copy link

@jainh jainh Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is your own unmarshal code ? it is being performed here inside the response/request class. I totally see scalability issue in this patch and not comfortable in giving thumbs up, What you need is single instance of context (which is a good thought) but in code you are doing it at two different places pretty much in a similar way just flipping class type. If you really need to parameterize class type, again create singleton class and initialize it with custom class type (request or response). But, don't patch or modify existing class with synchronized block.

IMHO, we should avoid synchronization, whenever possible and have things thread local and immutable.

synchronized (JaxpRequest.class) {
if(context == null) {
context = JAXBContext.newInstance(ResponseType.class);
}
}
}

Unmarshaller unmarshaller = context.createUnmarshaller();
JAXBElement<ResponseType> jaxbElementResponse = unmarshaller.unmarshal(nodeRoot,
ResponseType.class);
Expand Down