Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing of custom message attributes #69

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ public final XmlStringBuilder toXML() {
XmlStringBuilder buf = new XmlStringBuilder();
buf.halfOpenElement(IQ_ELEMENT);
addCommonAttributes(buf);
addCustomAttributes(buf);
Copy link
Member

Choose a reason for hiding this comment

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

I would merge addCommonattributes and addCustomAttributes, so just add the code in addCustomAttributes to the already existing method and rename it to addAttributes.

if (type == null) {
buf.attribute("type", "get");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ public XmlStringBuilder toXML() {
XmlStringBuilder buf = new XmlStringBuilder();
buf.halfOpenElement(ELEMENT);
addCommonAttributes(buf);
addCustomAttributes(buf);
buf.optAttribute("type", type);
buf.rightAngleBracket();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ public XmlStringBuilder toXML() {
XmlStringBuilder buf = new XmlStringBuilder();
buf.halfOpenElement(ELEMENT);
addCommonAttributes(buf);
addCustomAttributes(buf);
if (type != Type.available) {
buf.attribute("type", type);
}
Expand Down
161 changes: 161 additions & 0 deletions smack-core/src/main/java/org/jivesoftware/smack/packet/Stanza.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
import org.jxmpp.util.XmppStringUtils;

import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;

/**
* Base class for XMPP Stanzas, which are called Stanza(/Packet) in older versions of Smack (i.e. < 4.1).
Expand All @@ -55,12 +57,15 @@ public abstract class Stanza implements TopLevelStreamElement {
protected static final String DEFAULT_LANGUAGE =
java.util.Locale.getDefault().getLanguage().toLowerCase(Locale.US);

private static boolean customAttributesEnabled = false;

private final MultiMap<String, ExtensionElement> packetExtensions = new MultiMap<>();

private String id = null;
private Jid to;
private Jid from;
private XMPPError error = null;
private final LinkedHashMap<String, String> customAttributes = new LinkedHashMap<>();

/**
* Optional value of the 'xml:lang' attribute of the outermost element of
Expand Down Expand Up @@ -92,6 +97,11 @@ protected Stanza(Stanza p) {
for (ExtensionElement pe : p.getExtensions()) {
addExtension(pe);
}

if (isCustomAttributesEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to check here.

customAttributes.clear();
customAttributes.putAll(p.getCustomAttributes());
}
}

/**
Expand Down Expand Up @@ -458,6 +468,144 @@ public ExtensionElement removeExtension(ExtensionElement extension) {
return removeExtension(extension.getElementName(), extension.getNamespace());
}

/**
* Returns <tt>true</tt> if custom attributes management is enabled.
* @return <tt>true</tt> if custom attributes management is enabled.
*/
public static boolean isCustomAttributesEnabled() {
Copy link
Member

@Flowdalic Flowdalic Apr 14, 2016

Choose a reason for hiding this comment

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

Not sure, but maybe areCustomAttributesEnabled()? Or just customAttributesEnabled()?

return customAttributesEnabled;
}

/**
* Enables the possibility to add custom attributes to the stanza.
* <br/><b>Enabling this feature breaks the XMPP standard and may induce unexpected behaviors!
* <br/>If your use-case allow it, prefer using ExtensionElement instead, and if not, be sure to triple-check the result.</b>
*/
public static void enableCustomAttributes() {
customAttributesEnabled = true;
}

private static void requireCustomAttributesEnabled(String methodName) {
Copy link
Member

Choose a reason for hiding this comment

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

If you really want to the callers method name here, then get it via runtime mechanism instead of hardcoding it at the call site. All in all, I don't think it's really required, so just remove the methodName argument.

if (!isCustomAttributesEnabled()) {
throw new IllegalStateException(
"You cannot use " + methodName + " without enabling it first with enableCustomAttributes. " +
"Ensure you are aware of the consequences of doing so.");
}
}

/**
* Replaces all custom attributes of the stanza.
* @param attributes a map (name/value) of all the custom attributes of this stanza. SHould not be {@null}
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public void setCustomAttributes(Map<String, String> attributes) {
requireCustomAttributesEnabled("setCustomAttributes");
assert attributes != null;

synchronized (customAttributes) {
Copy link
Member

Choose a reason for hiding this comment

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

I really don't think we need to design this API thread safe. I know legacy Smack code does it here and there, but we shouldn't do it in this case.

customAttributes.clear();
customAttributes.putAll(attributes);
}
}

/**
* Adds a custom attribute to the stanza. If a custom attribute with the same name is already present, its value is updated.
* @param attributeName the custom attribute name.
* @param attributeValue the custom attribute value.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public void setCustomAttribute(String attributeName, String attributeValue) {
requireCustomAttributesEnabled("setCustomAttribute");
Copy link
Member

Choose a reason for hiding this comment

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

You need to check in the set* methods that the custom attribute name doesn't clash with a common attribute name to prevent users from producing invalid XML. Throw an IllegalArgumentException if so.

requireNotNullOrEmpty(attributeName, "attributeName must not be null or empty");

synchronized (customAttributes) {
customAttributes.put(attributeName, attributeValue);
}
}

/**
* Removes all custom attributes of the stanza.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public void removeCustomAttributes() {
requireCustomAttributesEnabled("removeCustomAttributes");

synchronized (customAttributes) {
customAttributes.clear();
}
}

/**
* Removes a custom attribute of the stanza if it exists.
* @param attributeName the custom attribute name.
* @return the removed custom attribute value or <tt>null</tt> if is was not present.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public String removeCustomAttribute(String attributeName) {
requireCustomAttributesEnabled("removeCustomAttribute");
requireNotNullOrEmpty(attributeName, "attributeName must not be null or empty");

synchronized (customAttributes) {
return customAttributes.remove(attributeName);
}
}

/**
* Returns <tt>true</tt> if this stanza contains custom attributes.
* @return <tt>true</tt> if this stanza contains custom attributes.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public boolean hasCustomAttributes() {
requireCustomAttributesEnabled("hasCustomAttributes");

synchronized (customAttributes) {
return !customAttributes.isEmpty();
}
}

/**
* Returns <tt>true</tt> if this stanza contains a custom attribute with this name.
* @param attributeName the custom attribute name.
* @return <tt>true</tt> if this stanza contains a custom attribute with this name.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public boolean hasCustomAttribute(String attributeName) {
requireCustomAttributesEnabled("hasCustomAttribute");
requireNotNullOrEmpty(attributeName, "attributeName must not be null or empty");

synchronized (customAttributes) {
return customAttributes.containsKey(attributeName);
}
}

/**
* Returns a map (name/value) of all the custom attributes of this stanza.
* @return a map (name/value) of all the custom attributes of this stanza.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public Map<String, String> getCustomAttributes() {
requireCustomAttributesEnabled("getCustomAttributes");

synchronized (customAttributes) {
return customAttributes;
Copy link
Member

@Flowdalic Flowdalic Apr 14, 2016

Choose a reason for hiding this comment

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

Unnecessary synchronized block btw. And since you need to validate the input in the set methods, you should return an unmodifiable Map here.

}
}

/**
* Returns the value of a custom attribute, or {@code null} if this stanza does not contain any custom attribute with this name.
* @param attributeName the custom attribute name.
* @return the value of a custom attribute, or {@code null} if this stanza does not contain any custom attribute with this name.
* @throws IllegalStateException if custom attributes management is not enabled.
*/
public String getCustomAttribute(String attributeName) {
requireCustomAttributesEnabled("getCustomAttribute");
requireNotNullOrEmpty(attributeName, "attributeName must not be null or empty");

synchronized (customAttributes) {
return customAttributes.get(attributeName);
}
}

@Override
// NOTE When Smack is using Java 8, then this method should be moved in Element as "Default Method".
public String toString() {
Expand Down Expand Up @@ -501,6 +649,19 @@ protected void addCommonAttributes(XmlStringBuilder xml) {
xml.xmllangAttribute(getLanguage());
}

/**
* Add custom attributes
*
* @param xml
*/
protected void addCustomAttributes(XmlStringBuilder xml) {
if(customAttributes != null) {
Copy link
Member

Choose a reason for hiding this comment

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

I would go for if (customAttributes == null) return; here, to avoid a level of indentation.

for (Map.Entry<String, String> entry : customAttributes.entrySet()) {
xml.optAttribute(entry.getKey(), entry.getValue());
Copy link
Member

Choose a reason for hiding this comment

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

Why optAttribute when we know that the attribute's value is not null?

}
}
}

/**
* Append an XMPPError is this stanza(/packet) has one set.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,10 @@ public static Message parseMessage(XmlPullParser parser)
defaultLanguage = Stanza.getDefaultLanguage();
}

if(Stanza.isCustomAttributesEnabled()) {
message.setCustomAttributes(ParserUtils.getCustomAttributes(parser));
}

// Parse sub-elements. We include extra logic to make sure the values
// are only read once. This is because it's possible for the names to appear
// in arbitrary sub-elements.
Expand Down Expand Up @@ -538,6 +542,10 @@ public static Presence parsePresence(XmlPullParser parser)
// CHECKSTYLE:ON
}

if(Stanza.isCustomAttributesEnabled()) {
presence.setCustomAttributes(ParserUtils.getCustomAttributes(parser));
}

// Parse sub-elements
outerloop: while (true) {
int eventType = parser.next();
Expand Down Expand Up @@ -611,6 +619,7 @@ public static IQ parseIQ(XmlPullParser parser) throws Exception {
final Jid to = ParserUtils.getJidAttribute(parser, "to");
final Jid from = ParserUtils.getJidAttribute(parser, "from");
final IQ.Type type = IQ.Type.fromString(parser.getAttributeValue("", "type"));
final Map<String, String> customAttributes = ParserUtils.getCustomAttributes(parser);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not conditional?


outerloop: while (true) {
int eventType = parser.next();
Expand Down Expand Up @@ -668,6 +677,9 @@ public static IQ parseIQ(XmlPullParser parser) throws Exception {
iqPacket.setFrom(from);
iqPacket.setType(type);
iqPacket.setError(error);
if (Stanza.isCustomAttributesEnabled()) {
iqPacket.setCustomAttributes(customAttributes);
}

return iqPacket;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.text.ParseException;
import java.util.Arrays;
import java.util.Date;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;

import org.jxmpp.jid.EntityBareJid;
import org.jxmpp.jid.Jid;
Expand Down Expand Up @@ -194,4 +198,17 @@ public static URI getUriFromNextText(XmlPullParser parser) throws XmlPullParserE
return uri;
}

public static Map<String, String> getCustomAttributes(XmlPullParser parser) {
Map<String, String> customAttributes = new LinkedHashMap<>();
List<String> standardAttributes = Arrays.asList(new String[] {"xml:lang", "id", "to", "from", "type"});
Copy link
Member

Choose a reason for hiding this comment

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

Why construct this every time the method is called? And why is it a List? This should be a static Set.

for (int i = 0; i < parser.getAttributeCount(); i++) {
String attributeName = parser.getAttributeName(i);
boolean isLangAttribute = "lang".equals(attributeName) && "xml".equals(parser.getAttributePrefix(i));
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this redundant with the xml:lang check because of standardAttributes?

Copy link
Author

Choose a reason for hiding this comment

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

I reused the logic I found in PacketParserUtils.getLanguageAttribute(). Do you think it's overkill in this case ?

if (!standardAttributes.contains(attributeName) && !isLangAttribute) {
String attributeValue = parser.getAttributeValue(i);
customAttributes.put(attributeName, attributeValue);
}
}
return customAttributes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
import org.jivesoftware.smack.sasl.packet.SaslStreamElements.SASLFailure;
import org.jivesoftware.smack.test.util.TestUtils;
import org.jivesoftware.smack.test.util.XmlUnitUtils;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.xml.sax.SAXException;
import org.xmlpull.v1.XmlPullParser;
import org.xmlpull.v1.XmlPullParserException;
Expand All @@ -49,6 +51,9 @@

public class PacketParserUtilsTest {

@Rule
public ExpectedException expectedException = ExpectedException.none();

private static Properties outputProperties = new Properties();
{
outputProperties.put(javax.xml.transform.OutputKeys.OMIT_XML_DECLARATION, "yes");
Expand Down Expand Up @@ -816,6 +821,41 @@ public void parseElementMultipleNamespace()
assertXMLEqual(stanza, result.toString());
}

@Test
public void parseMessageWithCustomAttributes() throws FactoryConfigurationError, Exception {
final String customAttrName = "customAttrName";
final String customAttrValue = "customAttrValue";

final String stanzaString = XMLBuilder.create("message")
.a("from", "romeo@montague.lit/orchard")
.a("to", "juliet@capulet.lit/balcony")
.a("id", "zid615d9")
.a("type", "chat")
.a(customAttrName, customAttrValue)
.a("xml:lang", Stanza.getDefaultLanguage())
.e("body")
.t("This is a test of the custom attributes parsing in message stanza")
.asString(outputProperties);

Stanza stanza = PacketParserUtils.parseStanza(PacketParserUtils.getParserFor(stanzaString));

// Should crash because feature was not enabled
expectedException.expect(IllegalStateException.class);
stanza.getCustomAttributes();

// Custom attributes not parsed because feature was enabled after the parsing
Stanza.enableCustomAttributes();
assertFalse(stanza.hasCustomAttributes());

// Parse again for getting custom attributes
stanza = PacketParserUtils.parseMessage(PacketParserUtils.getParserFor(stanzaString));
assertTrue(stanza.hasCustomAttributes());
assertTrue(stanza.getCustomAttributes().size() == 1);
assertTrue(stanza.hasCustomAttribute(customAttrName));
assertTrue(stanza.getCustomAttribute(customAttrName).equals(customAttrValue));
assertXMLEqual(stanzaString, stanza.toXML().toString());
}

@Test
public void parseSASLFailureSimple() throws FactoryConfigurationError, SAXException, IOException,
TransformerException, ParserConfigurationException, XmlPullParserException {
Expand Down