You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/05/17 21:21:07 UTC
[GitHub] [qpid-proton-j] tabish121 opened a new pull request #40: PROTON-2299 Support encode and decode with multiple body sections
tabish121 opened a new pull request #40:
URL: https://github.com/apache/qpid-proton-j/pull/40
Add some APIs in Message that allow for multiple sections in the message
body portion. Some tests added to validate the expectations of the new
APIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton-j] tabish121 commented on a change in pull request #40: PROTON-2299 Support encode and decode with multiple body sections
Posted by GitBox <gi...@apache.org>.
tabish121 commented on a change in pull request #40:
URL: https://github.com/apache/qpid-proton-j/pull/40#discussion_r634466429
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
Review comment:
I didn't intend to add that level of validation to the existing version of Message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton-j] gemmellr commented on a change in pull request #40: PROTON-2299 Support encode and decode with multiple body sections
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #40:
URL: https://github.com/apache/qpid-proton-j/pull/40#discussion_r634408811
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
Review comment:
I think its probably worth considering another interface than just Section for this method, to make clear it only applies to Data and AmqpSequence sections, as the only ones allowed multiple body sections. That, or if the type symmetry with getters is more desirable, the impl needs to validate the contents and the API doc call it out.
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
+
+ /**
+ * Adds the given {@link Section} to the internal collection of sections that will be sent
+ * to the remote peer when this message is encoded. If a previous section was added by a call
+ * to the {@link #setBody(Object)} method it should be retained as the first element of
Review comment:
setBody takes Section rather than Object.
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java
##########
@@ -514,7 +536,32 @@ public ApplicationProperties getApplicationProperties()
@Override
public Section getBody()
{
- return _body;
+ Section section = _body;
Review comment:
For clarity I might be inclined to null check and return this, like the next method does.
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
+
+ /**
+ * Adds the given {@link Section} to the internal collection of sections that will be sent
+ * to the remote peer when this message is encoded. If a previous section was added by a call
+ * to the {@link #setBody(Object)} method it should be retained as the first element of
+ * the running list of body sections contained in this message.
+ *
+ * @param bodySection
+ * The {@link Section} instance to append to the internal collection.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message addBodySection(Section bodySection);
+
+ /**
+ * Create and return a unmodifiable {@link Collection} that contains the {@link Section} instances currently
+ * assigned to this message. Changes to the returned Collection are not reflected in the Message.
+ *
+ * @return a {@link Collection} that is a copy of the current sections assigned to this message.
Review comment:
..or an empty list if none?
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
+
+ /**
+ * Adds the given {@link Section} to the internal collection of sections that will be sent
+ * to the remote peer when this message is encoded. If a previous section was added by a call
+ * to the {@link #setBody(Object)} method it should be retained as the first element of
+ * the running list of body sections contained in this message.
+ *
+ * @param bodySection
+ * The {@link Section} instance to append to the internal collection.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message addBodySection(Section bodySection);
Review comment:
Similar to above, but even additionally it could be restricted to only body type sections (the ship obviously sailed on the existing setBody mothod, but still may be worth tightening it up for this one)
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java
##########
@@ -557,6 +604,76 @@ public void setApplicationProperties(ApplicationProperties applicationProperties
public void setBody(Section body)
{
_body = body;
+ _bodySections = null;
+ }
+
+ @Override
+ public Message setBodySections(Collection<Section> sections)
+ {
+ clear();
+ if (sections != null && !sections.isEmpty())
+ {
+ _bodySections = new ArrayList<>(sections);
Review comment:
For consistency I wondered if this should maybe set _body if there is only 1 element in collection.
Related to earlier comments, should validate or restrict the types allowed.
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
+ * given replaces any existing section(s) assigned to this message through the {@link Message#setBody(Object)}
+ * or {@link #addBodySection(Section)} methods. Calling this method with a null
+ * or empty collection is equivalent to calling the {@link #clear()} method.
+ *
+ * @param sections
+ * The {@link Collection} of {@link Section} instance to assign this message.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message setBodySections(Collection<Section> sections);
+
+ /**
+ * Adds the given {@link Section} to the internal collection of sections that will be sent
+ * to the remote peer when this message is encoded. If a previous section was added by a call
+ * to the {@link #setBody(Object)} method it should be retained as the first element of
+ * the running list of body sections contained in this message.
+ *
+ * @param bodySection
+ * The {@link Section} instance to append to the internal collection.
+ *
+ * @return this {@link Message} instance.
+ */
+ Message addBodySection(Section bodySection);
+
+ /**
+ * Create and return a unmodifiable {@link Collection} that contains the {@link Section} instances currently
+ * assigned to this message. Changes to the returned Collection are not reflected in the Message.
Review comment:
If it is unmodifiable, no need to comment on changes to it.
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java
##########
@@ -557,6 +604,76 @@ public void setApplicationProperties(ApplicationProperties applicationProperties
public void setBody(Section body)
{
_body = body;
+ _bodySections = null;
+ }
+
+ @Override
+ public Message setBodySections(Collection<Section> sections)
+ {
+ clear();
+ if (sections != null && !sections.isEmpty())
+ {
+ _bodySections = new ArrayList<>(sections);
+ }
+
+ return this;
+ }
+
+ @Override
+ public Message addBodySection(Section bodySection)
+ {
+ Objects.requireNonNull(bodySection, "Additional Body Section cannot be null");
Review comment:
Note NPE or non-null requirement in javadoc?
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/Message.java
##########
@@ -210,4 +213,55 @@ public static Message create(Header header,
void clear();
MessageError getError();
+
+ /**
+ * @return the total number of body {@link Section} elements contained in this {@link Message}
+ */
+ int getBodySectionCount();
+
+ /**
+ * Sets the body {@link Section} instances to use when encoding this message. The value
Review comment:
Perhaps note that the changes to the collection afterwards aren't reflected in the message?
##########
File path: proton-j/src/main/java/org/apache/qpid/proton/message/impl/MessageImpl.java
##########
@@ -666,9 +784,9 @@ public void decode(ReadableBuffer buffer)
}
}
- if(section != null && !(section instanceof Footer))
+ while(section != null && !(section instanceof Footer))
{
- _body = section;
+ addBodySection(section);
Review comment:
This should fail if receiving multiple body sections with differing types (or multiple AmqpValue sections)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton-j] tabish121 closed pull request #40: PROTON-2299 Support encode and decode with multiple body sections
Posted by GitBox <gi...@apache.org>.
tabish121 closed pull request #40:
URL: https://github.com/apache/qpid-proton-j/pull/40
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton-j] tabish121 commented on pull request #40: PROTON-2299 Support encode and decode with multiple body sections
Posted by GitBox <gi...@apache.org>.
tabish121 commented on pull request #40:
URL: https://github.com/apache/qpid-proton-j/pull/40#issuecomment-843246831
Closing this one as the scope of adding mixed validation into the currently lax Message implementation feels out of place and if that's the intended result then I'd suggest rather adding a completely new Message API that meets the full breadth of the AMQP compliance checking desired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org