Apologies for the delayed response, I had a few other issues that
distracted my from this task...
John Dennis wrote:
On 07/16/2009 07:03 AM, Gordon Sim wrote:
John,
I agree with every point you make. The FieldTable and Array classes are
pretty horrible to use except for the simple cases and lack consistency
in design. They also expose boost which we are trying to move away
from[1] and tie the encoding details to the data structure[2] and
application visible API. In short, though we will obviously continue to
support it for existing users, I don't think that FieldTable in its
current form is really suitable as part of the exposed API in the long
term.
Thanks for the follow-up Gordon. I think you answered my high order bit
question. Do we fix what we have or do we re-implement it?
I've gone back and forth on that question. I think we need to keep
FieldTable, and ideally not break existing applications or ABI.
What I've done so far is to define a new Variant type and have maps and
lists of those. This is part of a proposed higher level api for
messaging (see separate mail).
In terms of the implementation I've currently mapped these onto
FieldValues for encode/decode so that I can use the existing 0-10 client
code beneath the new api. Doing this I hit all the issues you discussed
and so have modified FieldValue and FieldTable to address them (at least
to a degree). I have also add a List type corresponding to the amqp0-10
list.
I was going
to offer to send a set of patches for the current implementation but in
a conversation with Ted Ross he cautioned patience because of the
possibility this area of the API might be reworked.
Attached is a patch that contains just the changes to FieldTable and
FieldValue, plus the addition of the List class.
It's good to know
there is a consensus the existing implementation needs some love and I
wasn't just out in left field somewhere due to misunderstandings.
No, it's definitely an area in need of some attention! My view is still
that the best solution is an alternative api however as described I've
ended up wanting to make some additions to the existing api as well (I
believe these are all backwards compatible).
Some time ago the desirability of a higher level API for the c++ client
was discussed and I started some work on that. For the past couple of
months my attention has been diverted by other work but I hope to return
to it now. This API will include a variant type and a corresponding map
of these through which message properties would be accessed. The
intention is also to provide direct support for messages whose content
is an encoded form of a map or list. I still have some details to work
through of course, including how to avoid breaking the existing API
while avoiding expensive conversions.
At the moment I'm fine with waiting for the new implementation, I can
work around the issues from my end. I'll look forward to a cleaner
design when you have it ready. If you would like to bounce some design
ideas off me I'd be happy to provide feedback, or if there is a design
proposal on a wiki or some such you might want to provide a pointer to
that works as well.
That would be great! You can have a look at the framing namespace
changes in the attached patch (unless anyone objects I'll probably check
this part in shortly); for the alternative see the other mail on the
messaging api.
--Gordon.
Index: include/qpid/framing/FieldTable.h
===================================================================
--- include/qpid/framing/FieldTable.h (revision 799643)
+++ include/qpid/framing/FieldTable.h (working copy)
@@ -51,6 +51,8 @@
typedef boost::shared_ptr<FieldValue> ValuePtr;
typedef std::map<std::string, ValuePtr> ValueMap;
typedef ValueMap::iterator iterator;
+ typedef ValueMap::const_reference const_reference;
+ typedef ValueMap::value_type value_type;
QPID_COMMON_EXTERN FieldTable() {};
QPID_COMMON_EXTERN FieldTable(const FieldTable& ft);
@@ -97,12 +99,16 @@
QPID_COMMON_EXTERN bool operator==(const FieldTable& other) const;
// Map-like interface.
- // TODO: may need to duplicate into versions that return mutable iterator
ValueMap::const_iterator begin() const { return values.begin(); }
ValueMap::const_iterator end() const { return values.end(); }
ValueMap::const_iterator find(const std::string& s) const { return values.find(s); }
+ ValueMap::iterator begin() { return values.begin(); }
+ ValueMap::iterator end() { return values.end(); }
+ ValueMap::iterator find(const std::string& s) { return values.find(s); }
+
std::pair <ValueMap::iterator, bool> insert(const ValueMap::value_type&);
+ ValueMap::iterator insert(ValueMap::iterator, const ValueMap::value_type&);
void clear() { values.clear(); }
// ### Hack Alert
Index: include/qpid/framing/List.h
===================================================================
--- include/qpid/framing/List.h (revision 0)
+++ include/qpid/framing/List.h (revision 0)
@@ -0,0 +1,75 @@
+#ifndef QPID_FRAMING_LIST_H
+#define QPID_FRAMING_LIST_H
+
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include "qpid/CommonImportExport.h"
+#include <iostream>
+#include <list>
+#include <boost/shared_ptr.hpp>
+
+namespace qpid {
+namespace framing {
+
+class Buffer;
+class FieldValue;
+
+/**
+ * Representation of an AMQP 0-10 list
+ */
+class List
+{
+ public:
+ typedef boost::shared_ptr<FieldValue> ValuePtr;
+ typedef std::list<ValuePtr> Values;
+ typedef Values::const_iterator const_iterator;
+ typedef Values::iterator iterator;
+ typedef Values::const_reference const_reference;
+
+ QPID_COMMON_EXTERN uint32_t encodedSize() const;
+ QPID_COMMON_EXTERN void encode(Buffer& buffer) const;
+ QPID_COMMON_EXTERN void decode(Buffer& buffer);
+
+ QPID_COMMON_EXTERN bool operator==(const List& other) const;
+
+ // std collection interface.
+ QPID_COMMON_EXTERN const_iterator begin() const { return values.begin(); }
+ QPID_COMMON_EXTERN const_iterator end() const { return values.end(); }
+ QPID_COMMON_EXTERN iterator begin() { return values.begin(); }
+ QPID_COMMON_EXTERN iterator end(){ return values.end(); }
+
+ QPID_COMMON_EXTERN ValuePtr front() const { return values.front(); }
+ QPID_COMMON_EXTERN ValuePtr back() const { return values.back(); }
+ QPID_COMMON_EXTERN size_t size() const { return values.size(); }
+
+ QPID_COMMON_EXTERN iterator insert(iterator i, ValuePtr value) { return values.insert(i, value); }
+ QPID_COMMON_EXTERN void erase(iterator i) { values.erase(i); }
+ QPID_COMMON_EXTERN void push_back(ValuePtr value) { values.insert(end(), value); }
+ QPID_COMMON_EXTERN void pop_back() { values.pop_back(); }
+
+ private:
+ Values values;
+
+ friend QPID_COMMON_EXTERN std::ostream& operator<<(std::ostream& out, const List& list);
+};
+}} // namespace qpid::framing
+
+#endif /*!QPID_FRAMING_LIST_H*/
Property changes on: include/qpid/framing/List.h
___________________________________________________________________
Name: svn:keywords
+ Rev Date
Name: svn:eol-style
+ native
Index: include/qpid/framing/FieldValue.h
===================================================================
--- include/qpid/framing/FieldValue.h (revision 799643)
+++ include/qpid/framing/FieldValue.h (working copy)
@@ -24,6 +24,7 @@
#include "qpid/Exception.h"
#include "qpid/framing/amqp_types.h"
#include "qpid/framing/Buffer.h"
+#include "qpid/framing/Endian.h"
#include "qpid/framing/FieldTable.h"
#include "qpid/CommonImportExport.h"
@@ -36,7 +37,6 @@
namespace qpid {
namespace framing {
-//class Array;
/**
* Exception that is the base exception for all field table errors.
*
@@ -53,6 +53,8 @@
InvalidConversionException() {}
};
+class List;
+
/**
* Value that can appear in an AMQP field table
*
@@ -96,12 +98,17 @@
template <typename T> bool convertsTo() const { return false; }
template <typename T> T get() const { throw InvalidConversionException(); }
+ template <class T, int W> T getIntegerValue() const;
+ template <class T, int W> T getFloatingPointValue() const;
+ template <class T> bool get(T&) const;
+
protected:
FieldValue(uint8_t t, Data* d): typeOctet(t), data(d) {}
private:
uint8_t typeOctet;
std::auto_ptr<Data> data;
+
};
template <>
@@ -165,11 +172,53 @@
return v;
}
uint8_t* rawOctets() { return octets; }
+ uint8_t* const rawOctets() const { return octets; }
void print(std::ostream& o) const { o << "F" << width << ":"; };
};
+template <class T, int W>
+inline T FieldValue::getIntegerValue() const
+{
+ FixedWidthValue<W>* const fwv = dynamic_cast< FixedWidthValue<W>* const>(data.get());
+ if (fwv) {
+ uint8_t* octets = fwv->rawOctets();
+ T v = 0;
+ for (int i = 0; i < W-1; ++i) {
+ v |= octets[i]; v <<= 8;
+ }
+ v |= octets[W-1];
+ return v;
+ } else {
+ throw InvalidConversionException();
+ }
+}
+
+template <class T, int W>
+inline T FieldValue::getFloatingPointValue() const {
+ FixedWidthValue<W>* const fwv = dynamic_cast< FixedWidthValue<W>* const>(data.get());
+ if (fwv) {
+ T value;
+ uint8_t* const octets = Endian::convertIfRequired(fwv->rawOctets(), W);
+ uint8_t* const target = reinterpret_cast<uint8_t*>(&value);
+ for (uint i = 0; i < W; ++i) target[i] = octets[i];
+ return value;
+ } else {
+ throw InvalidConversionException();
+ }
+}
+
template <>
+inline float FieldValue::get<float>() const {
+ return getFloatingPointValue<float, 4>();
+}
+
+template <>
+inline double FieldValue::get<double>() const {
+ return getFloatingPointValue<double, 8>();
+}
+
+template <>
class FixedWidthValue<0> : public FieldValue::Data {
public:
// Implicit default constructor is fine
@@ -243,6 +292,27 @@
void print(std::ostream& o) const { o << "[" << value << "]"; };
};
+/**
+ * Accessor that can be used to get values of type FieldTable, Array
+ * and List.
+ */
+template <class T>
+inline bool FieldValue::get(T& t) const
+{
+ const EncodedValue<T>* v = dynamic_cast< EncodedValue<T>* >(data.get());
+ if (v != 0) {
+ t = v->getValue();
+ return true;
+ } else {
+ try {
+ t = get<T>();
+ return true;
+ } catch (const InvalidConversionException&) {
+ return false;
+ }
+ }
+}
+
class Str8Value : public FieldValue {
public:
QPID_COMMON_EXTERN Str8Value(const std::string& v);
@@ -294,6 +364,7 @@
class FieldTableValue : public FieldValue {
public:
+ typedef FieldTable ValueType;
QPID_COMMON_EXTERN FieldTableValue(const FieldTable&);
};
@@ -302,6 +373,49 @@
QPID_COMMON_EXTERN ArrayValue(const Array&);
};
+class VoidValue : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN VoidValue();
+};
+
+class BoolValue : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN BoolValue(bool);
+};
+
+class Unsigned8Value : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN Unsigned8Value(uint8_t);
+};
+
+class Unsigned16Value : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN Unsigned16Value(uint16_t);
+};
+
+class Unsigned32Value : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN Unsigned32Value(uint32_t);
+};
+
+class Integer8Value : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN Integer8Value(int8_t);
+};
+
+class Integer16Value : public FieldValue {
+ public:
+ QPID_COMMON_EXTERN Integer16Value(int16_t);
+};
+
+typedef IntegerValue Integer32Value;
+
+class ListValue : public FieldValue {
+ public:
+ typedef List ValueType;
+ QPID_COMMON_EXTERN ListValue(const List&);
+};
+
template <class T>
bool getEncodedValue(FieldTable::ValuePtr vptr, T& value)
{
@@ -315,7 +429,6 @@
return false;
}
-
}} // qpid::framing
#endif
Index: src/qpid/framing/List.cpp
===================================================================
--- src/qpid/framing/List.cpp (revision 0)
+++ src/qpid/framing/List.cpp (revision 0)
@@ -0,0 +1,83 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include "qpid/framing/List.h"
+#include "qpid/framing/Buffer.h"
+#include "qpid/framing/FieldValue.h"
+#include "qpid/Exception.h"
+#include "qpid/framing/reply_exceptions.h"
+
+namespace qpid {
+namespace framing {
+
+uint32_t List::encodedSize() const
+{
+ uint32_t len(4/*size*/ + 4/*count*/);
+ for(Values::const_iterator i = values.begin(); i != values.end(); ++i) {
+ len += (*i)->encodedSize();
+ }
+ return len;
+}
+
+void List::encode(Buffer& buffer) const
+{
+ buffer.putLong(encodedSize() - 4);
+ buffer.putLong(size());
+ for (Values::const_iterator i = values.begin(); i!=values.end(); ++i) {
+ (*i)->encode(buffer);
+ }
+}
+
+void List::decode(Buffer& buffer)
+{
+ values.clear();
+ uint32_t size = buffer.getLong();
+ uint32_t available = buffer.available();
+ if (available < size) {
+ throw IllegalArgumentException(QPID_MSG("Not enough data for list, expected "
+ << size << " bytes but only " << available << " available"));
+ }
+ if (size) {
+ uint32_t count = buffer.getLong();
+ for (uint32_t i = 0; i < count; i++) {
+ ValuePtr value(new FieldValue);
+ value->decode(buffer);
+ values.push_back(value);
+ }
+ }
+}
+
+
+bool List::operator==(const List& other) const {
+ return values.size() == other.values.size() &&
+ std::equal(values.begin(), values.end(), other.values.begin());
+}
+
+std::ostream& operator<<(std::ostream& out, const List& l)
+{
+ out << "{";
+ for(List::Values::const_iterator i = l.values.begin(); i != l.values.end(); ++i) {
+ if (i != l.values.begin()) out << ", ";
+ (*i)->print(out);
+ }
+ return out << "}";
+}
+
+}} // namespace qpid::framing
Property changes on: src/qpid/framing/List.cpp
___________________________________________________________________
Name: svn:keywords
+ Rev Date
Name: svn:eol-style
+ native
Index: src/qpid/framing/FieldTable.cpp
===================================================================
--- src/qpid/framing/FieldTable.cpp (revision 799643)
+++ src/qpid/framing/FieldTable.cpp (working copy)
@@ -185,13 +185,8 @@
bool getRawFixedWidthValue(FieldTable::ValuePtr vptr, T& value)
{
if (vptr && vptr->getType() == typecode) {
- FixedWidthValue<width>* fwv = dynamic_cast< FixedWidthValue<width>* >(&vptr->getData());
- if (fwv) {
- uint8_t* const octets = Endian::convertIfRequired(fwv->rawOctets(), width);
- uint8_t* const target = reinterpret_cast<uint8_t*>(&value);
- for (uint i = 0; i < width; ++i) target[i] = octets[i];
- return true;
- }
+ value = vptr->get<T>();
+ return true;
}
return false;
}
@@ -370,5 +365,16 @@
values.erase(name);
}
+std::pair<FieldTable::ValueMap::iterator, bool> FieldTable::insert(const ValueMap::value_type& value)
+{
+ return values.insert(value);
}
+
+FieldTable::ValueMap::iterator FieldTable::insert(ValueMap::iterator position, const ValueMap::value_type& value)
+{
+ return values.insert(position, value);
}
+
+
+}
+}
Index: src/qpid/framing/Endian.h
===================================================================
--- src/qpid/framing/Endian.h (revision 799643)
+++ src/qpid/framing/Endian.h (working copy)
@@ -1,46 +0,0 @@
-#ifndef QPID_FRAMING_ENDIAN_H
-#define QPID_FRAMING_ENDIAN_H
-
-/*
- *
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- *
- */
-
-#include "qpid/sys/IntegerTypes.h"
-
-namespace qpid {
-namespace framing {
-
-/**
- * Conversion utility for little-endian platforms that need to convert
- * to and from network ordered octet sequences
- */
-class Endian
-{
- public:
- static uint8_t* convertIfRequired(uint8_t* const octets, int width);
- private:
- const bool littleEndian;
- Endian();
- static const Endian instance;
- static bool testBigEndian();
-};
-}} // namespace qpid::framing
-
-#endif /*!QPID_FRAMING_ENDIAN_H*/
Index: src/qpid/framing/FieldValue.cpp
===================================================================
--- src/qpid/framing/FieldValue.cpp (revision 799643)
+++ src/qpid/framing/FieldValue.cpp (working copy)
@@ -22,6 +22,7 @@
#include "qpid/framing/Array.h"
#include "qpid/framing/Buffer.h"
#include "qpid/framing/Endian.h"
+#include "qpid/framing/List.h"
#include "qpid/framing/reply_exceptions.h"
namespace qpid {
@@ -37,6 +38,8 @@
typeOctet = type;
if (typeOctet == 0xA8) {
data.reset(new EncodedValue<FieldTable>());
+ } else if (typeOctet == 0xA9) {
+ data.reset(new EncodedValue<List>());
} else if (typeOctet == 0xAA) {
data.reset(new EncodedValue<Array>());
} else {
@@ -164,10 +167,37 @@
{
}
+ListValue::ListValue(const List& l) : FieldValue(0xa9, new EncodedValue<List>(l))
+{
+}
+
ArrayValue::ArrayValue(const Array& a) : FieldValue(0xaa, new EncodedValue<Array>(a))
{
}
+VoidValue::VoidValue() : FieldValue(0xf0, new FixedWidthValue<0>()) {}
+
+BoolValue::BoolValue(bool b) :
+ FieldValue(0x08, new FixedWidthValue<1>(b))
+{}
+
+Unsigned8Value::Unsigned8Value(uint8_t v) :
+ FieldValue(0x02, new FixedWidthValue<1>(v))
+{}
+Unsigned16Value::Unsigned16Value(uint16_t v) :
+ FieldValue(0x12, new FixedWidthValue<2>(v))
+{}
+Unsigned32Value::Unsigned32Value(uint32_t v) :
+ FieldValue(0x22, new FixedWidthValue<4>(v))
+{}
+
+Integer8Value::Integer8Value(int8_t v) :
+ FieldValue(0x01, new FixedWidthValue<1>(v))
+{}
+Integer16Value::Integer16Value(int16_t v) :
+ FieldValue(0x11, new FixedWidthValue<2>(v))
+{}
+
void FieldValue::print(std::ostream& out) const {
data->print(out);
out << TypeCode(typeOctet) << '(';
Index: src/tests/FieldTable.cpp
===================================================================
--- src/tests/FieldTable.cpp (revision 799643)
+++ src/tests/FieldTable.cpp (working copy)
@@ -22,6 +22,7 @@
#include "qpid/framing/Array.h"
#include "qpid/framing/FieldTable.h"
#include "qpid/framing/FieldValue.h"
+#include "qpid/framing/List.h"
#include "qpid/sys/alloca.h"
#include "unit_test.h"
@@ -86,7 +87,9 @@
QPID_AUTO_TEST_CASE(testNestedValues)
{
- char buff[100];
+ double d = 1.2345;
+ uint32_t u = 101;
+ char buff[1000];
{
FieldTable a;
FieldTable b;
@@ -94,11 +97,17 @@
items.push_back("one");
items.push_back("two");
Array c(items);
+ List list;
+ list.push_back(List::ValuePtr(new Str16Value("red")));
+ list.push_back(List::ValuePtr(new Unsigned32Value(u)));
+ list.push_back(List::ValuePtr(new Str8Value("yellow")));
+ list.push_back(List::ValuePtr(new DoubleValue(d)));
a.setString("id", "A");
b.setString("id", "B");
a.setTable("B", b);
a.setArray("C", c);
+ a.set("my-list", FieldTable::ValuePtr(new ListValue(list)));
Buffer wbuffer(buff, 100);
@@ -119,6 +128,27 @@
BOOST_CHECK((uint) 2 == items.size());
BOOST_CHECK(string("one") == items[0]);
BOOST_CHECK(string("two") == items[1]);
+
+ List list;
+ BOOST_CHECK(a.get("my-list")->get<List>(list));
+ List::const_iterator i = list.begin();
+ BOOST_CHECK(i != list.end());
+ BOOST_CHECK_EQUAL(std::string("red"), (*i)->get<std::string>());
+
+ i++;
+ BOOST_CHECK(i != list.end());
+ BOOST_CHECK_EQUAL(u, (uint32_t) (*i)->get<int>());
+
+ i++;
+ BOOST_CHECK(i != list.end());
+ BOOST_CHECK_EQUAL(std::string("yellow"), (*i)->get<std::string>());
+
+ i++;
+ BOOST_CHECK(i != list.end());
+ BOOST_CHECK_EQUAL(d, (*i)->get<double>());
+
+ i++;
+ BOOST_CHECK(i == list.end());
}
}
Index: src/Makefile.am
===================================================================
--- src/Makefile.am (revision 799643)
+++ src/Makefile.am (working copy)
@@ -362,7 +362,6 @@
qpid/framing/Buffer.cpp \
qpid/framing/ChannelHandler.h \
qpid/framing/Endian.cpp \
- qpid/framing/Endian.h \
qpid/framing/FieldTable.cpp \
qpid/framing/FieldValue.cpp \
qpid/framing/FrameDecoder.cpp \
@@ -376,6 +375,7 @@
qpid/framing/InitiationHandler.h \
qpid/framing/InputHandler.h \
qpid/framing/Invoker.h \
+ qpid/framing/List.cpp \
qpid/framing/MethodBodyFactory.h \
qpid/framing/MethodContent.h \
qpid/framing/ModelMethod.h \
@@ -723,8 +723,10 @@
../include/qpid/client/TypedResult.h \
../include/qpid/framing/Array.h \
../include/qpid/framing/Buffer.h \
+ ../include/qpid/framing/Endian.h \
../include/qpid/framing/FieldTable.h \
../include/qpid/framing/FieldValue.h \
+ ../include/qpid/framing/List.h \
../include/qpid/framing/ProtocolVersion.h \
../include/qpid/framing/SequenceNumber.h \
../include/qpid/framing/SequenceSet.h \
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
