Servicebus fix for #406 #515

Merged
merged 2 commits into from Jun 22, 2012

Conversation

Projects
None yet
3 participants
Contributor

gcheng commented Jun 22, 2012

No description provided.

@jcookems jcookems commented on the diff Jun 22, 2012

WindowsAzure/ServiceBus/ServiceBusRestProxy.php
@@ -123,6 +123,9 @@ public function sendMessage($path, $brokeredMessage)
if (!empty($customProperties)) {
foreach ($customProperties as $key => $value) {
+ if (is_string($value)) {
@jcookems

jcookems Jun 22, 2012

Contributor

Need to include a serialization for dates and booleans too. See my sample code in #406:

} else if (is_bool($value)) {
    return ($value ? 'true' : 'false');
} else if ($value instanceof \DateTime) {
    $formatted = gmdate(Resources::AZURE_DATE_FORMAT, $value->getTimestamp());
    return '"' . $formatted . '"';
@jcookems

jcookems Jun 22, 2012

Contributor

Otherwise get this error when passing a date as a custom property:

WindowsAzure\Common\Internal\InvalidArgumentTypeException: The provided variable 'value' should be of type 'string'
...\WindowsAzure\Common\Internal\Validate.php:74
...\WindowsAzure\Common\Internal\Http\HttpCallContext.php:301
...\WindowsAzure\ServiceBus\ServiceBusRestProxy.php:129
...\WindowsAzure\ServiceBus\ServiceBusRestProxy.php:149
@jcookems

jcookems Jun 22, 2012

Contributor

If the serialization format is JSON, could use this instead:

    if (!empty($customProperties)) {
        foreach ($customProperties as $key => $value) {
            if ($value instanceof \DateTime) {
                $value = gmdate(Resources::AZURE_DATE_FORMAT, $value->getTimestamp());
            }
            $value = json_encode($value);
            $httpCallContext->addHeader($key, $value);
        }
    }

@jcookems jcookems and 1 other commented on an outdated diff Jun 22, 2012

WindowsAzure/ServiceBus/ServiceBusRestProxy.php
@@ -234,9 +237,13 @@ public function receiveMessage($path, $receiveMessageOptions = null)
$brokeredMessage->setBody($response->getBody());
foreach (array_keys($responseHeaders) as $headerKey) {
+ $value = $responseHeaders[$headerKey];
+ if (preg_match('/^\".*\"$/', $value)) {
+ $value = preg_replace('/^\"(.*)\"$/', '${1}', $value);
+ }
$brokeredMessage->setProperty(
@jcookems

jcookems Jun 22, 2012

Contributor

Need to handle dates and boolean too.

@jcookems

jcookems Jun 22, 2012

Contributor

In the "if" statement above, can add this code (from my sample in #406)

    $dateValue = Utilities::rfc1123ToDateTime($value);
    if (!$dateValue ) {
        $value = $dateValue ;
    }

The boolean is also easily handled:

} else if ('true' == $value) {
    $value = true;
} else if ('false' == $value) {
    $value = false;
@jcookems

jcookems Jun 22, 2012

Contributor

It does not look like int and floats are converted to their respective types; it looks like they are left as strings. This means that if the properties are read out and assigned to a new message, they will be sent to the server as strings the next time, changing their semantics.

Perhaps you can use your regular expressions to interpret only digits as integers, and only digits with a decimal as floats. Won't be perfect, but should get the 90%.

@jcookems

jcookems Jun 22, 2012

Contributor

If the serialization format is JSON, could use this instead:

        foreach ($responseHeaders as $headerKey => $value) {
            $decodedValueArray = (array)json_decode($value);
            if (count($decodedValueArray) == 1 && array_key_exists(0, $decodedValueArray)) {
                $decodedValue = $decodedValueArray[0];
                if (is_string($decodedValue)) {
                    $dateValue = Utilities::rfc1123ToDateTime($decodedValue);
                    if ($dateValue !== false) {
                        $decodedValue = $dateValue ;
                    }
                }                    
                $brokeredMessage->setProperty(
                    $headerKey, 
                    $decodedValue
                );
            }
        }
@gcheng

gcheng Jun 22, 2012

Contributor

This is becoming design discussion now. We need to consider the use cases and scenarios before diving into details. Here is a list of the scenarios. We should encode the right logic at the right layer of the SDK.
a. the sender sends a basic type such as integer, and the receive can unambiguously receive and decode it.
b. the sender sends a basic PHP type, the receiver cannot always correctly decode it without some additional information, such as boolean, or double.
c. the sender sends an extended PHP type such as DateTime, the receiver cannot always correctly decode it correctly without additional information.
d. the sender sends some custom data such as Money, etc, it is impossible to decode it correctly without prior knowledge of the data.

@jcookems jcookems commented on an outdated diff Jun 22, 2012

...t/WindowsAzure/ServiceBus/ServiceBusRestProxyTest.php
@@ -967,7 +967,7 @@ public function testMessageMayHaveCustomProperties()
$this->safeDeleteQueue($queueName);
$this->createQueue($queueInfo);
$brokeredMessage = new BrokeredMessage();
- $brokeredMessage->setProperty('hello', '"world"');
+ $brokeredMessage->setProperty('hello', 'world');
$brokeredMessage->setProperty('foo', 42);
$this->restProxy->sendQueueMessage($queueName, $brokeredMessage);
@jcookems

jcookems Jun 22, 2012

Contributor

Should test all supported types here: float (3.5), boolean (true, false) and date (new \DateTime('06/21/2012'))

@ogail ogail and 1 other commented on an outdated diff Jun 22, 2012

WindowsAzure/ServiceBus/ServiceBusRestProxy.php
@@ -234,9 +237,13 @@ public function receiveMessage($path, $receiveMessageOptions = null)
$brokeredMessage->setBody($response->getBody());
foreach (array_keys($responseHeaders) as $headerKey) {
+ $value = $responseHeaders[$headerKey];
+ if (preg_match('/^\".*\"$/', $value)) {
+ $value = preg_replace('/^\"(.*)\"$/', '${1}', $value);
@ogail

ogail Jun 22, 2012

Contributor

Nit: it would be good to have this regex in Resources with little comment about what is does

@jcookems

jcookems Jun 22, 2012

Contributor

Consider: Moving the serialization/deserialization code into helper methods on the Util class. That will make the feature more unit-testable.

@gcheng gcheng pushed a commit that referenced this pull request Jun 22, 2012

Albert Cheng Merge pull request #515 from gcheng/servicebus
Servicebus fix for #406
0ae7869

@gcheng gcheng merged commit 0ae7869 into Azure:dev Jun 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment