-
Notifications
You must be signed in to change notification settings - Fork 885
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
2aab2d7
f6c7f8c
5ee6f69
6aeee96
805f6ed
06136d0
c990bf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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). | ||
|
@@ -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 | ||
|
@@ -92,6 +97,11 @@ protected Stanza(Stanza p) { | |
for (ExtensionElement pe : p.getExtensions()) { | ||
addExtension(pe); | ||
} | ||
|
||
if (isCustomAttributesEnabled()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to check here. |
||
customAttributes.clear(); | ||
customAttributes.putAll(p.getCustomAttributes()); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, but maybe |
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary |
||
} | ||
} | ||
|
||
/** | ||
* 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() { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would go for |
||
for (Map.Entry<String, String> entry : customAttributes.entrySet()) { | ||
xml.optAttribute(entry.getKey(), entry.getValue()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||
} | ||
} | ||
} | ||
|
||
/** | ||
* Append an XMPPError is this stanza(/packet) has one set. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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(); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not conditional? |
||
|
||
outerloop: while (true) { | ||
int eventType = parser.next(); | ||
|
@@ -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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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"}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
for (int i = 0; i < parser.getAttributeCount(); i++) { | ||
String attributeName = parser.getAttributeName(i); | ||
boolean isLangAttribute = "lang".equals(attributeName) && "xml".equals(parser.getAttributePrefix(i)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this redundant with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reused the logic I found in |
||
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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"); | ||
|
@@ -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", "[email protected]/orchard") | ||
.a("to", "[email protected]/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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would merge
addCommonattributes
andaddCustomAttributes
, so just add the code inaddCustomAttributes
to the already existing method and rename it toaddAttributes
.