AMQNET-625: Content Property of IBytesMessage should be idempotent

[Havret]

Content Property getter should not generate side effects.

@cjwmorgan-sol could you please take a look at it?

[michaelandrepearce]

Can you add a test where when byte message is 24 bytes long, and three read calls with a dest byte array is size 8 bytes. Expected out come is first read returns first 8 bytes aka bytes in position 0-7, second read returns bytes position 8-13 , and last read returns bytes in position 14 to 23. Test should include a further read which obviously would not return data as no further bytes to be read unless message is reset

[Havret]

@michaelandrepearce Done. Moreover I've changed slightly the name of the first test to make it more descriptive.

The other thing is that I am not sure what should be the expected behavior of Content setter when somebody starts messing with Write* methods. Content property is a peculiar thing for NMS IBytesMessage and there is no equivalent of it in JMS API, thus it would be a futile attempt to look for inspiration in qpid-jms, as I used to do to tackle previous problems.

My implementation mimics what was done in NMS MSMQ:

https://github.com/apache/activemq-nms-msmq/blob/b6d954d9a97f11459f1b34956aeae675e456dfdd/src/main/csharp/BaseMessage.cs#L42-L47

where Content Property circumvents all the bits related to Binary Writers and Readers and exposes direct access to underlying message body.

@cjwmorgan-sol, @gemmellr, @michaelandrepearce What do you think?

[cjwmorgan-sol]

Currently modifying the return byte array reference modifies the underlying message byte array from message.Content is this desired? Or are you looking for a block copy of the Content when using get? And set?

[Havret]

@cjwmorgan-sol You are correct. I do return direct reference. TBH I don't know what is the expected behavior. Even if we allow mutating the underlying message representation, user always operates on deep copy of the message.

[cjwmorgan-sol]

Sounds good to me. It might be worth adding a comment describing the behaviour implemented for Content set and get for bytes messages though. Where get allows read write access to the buffer's bytes and set allows modifying the Buffer (ie size), or something along those lines. Also that "user always operates on deep copy of the message". Otherwise developers have to another layer deep (amqpnetlite) to find out the byte array is safe.

[michaelandrepearce]

So a message can only ever be in write or read mode. There are details in the spec around this and some tests that exist. Ill try dig it out and pass to you offline

[alaendle]

I "rediscovered" this issue (also can be found here: https://issues.apache.org/jira/browse/AMQNET-625), can anything be done to get the PR incorporated? The current behaviour is really annoying if you play around in test code with .Content.Length and afterwards take a look at the .Content and only see a lot of nulls 🤦‍♂️. For sure it is bad style to re-access/copy the underlying byte array, but from an equational reasoning standpoint it is hard to explain why var x = msg.Content; var l = msg.Content.Length; leads to a different result than swapping the statements var l = msg.Content.Length; var x = msg.Content; 😄

[Copilot]

Pull Request Overview

This PR makes the Content property of IBytesMessage idempotent by switching to a property-backed approach, refactoring facades and the message class, and adding tests to verify multiple reads.

• Introduce a Content getter/setter in both test and AMQP facade/message classes to eliminate stream side effects.
• Update tests to confirm that Content can be obtained repeatedly and that sequential reads behave correctly.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/Apache-NMS-AMQP-Test/Message/NmsBytesMessageTest.cs	Added tests for multiple Content retrieval and sequential reads
test/Apache-NMS-AMQP-Test/Message/Facade/NmsTestBytesMessageFacade.cs	Replaced private content field with Content property usage
src/NMS.AMQP/Provider/Amqp/Message/AmqpNmsBytesMessageFacade.cs	Refactored to use Content property; removed GetBinaryFromBody
src/NMS.AMQP/Message/NmsBytesMessage.cs	Updated Content to delegate to facade; extracted ReadBytes helper
src/NMS.AMQP/Message/Facade/INmsBytesMessageFacade.cs	Added Content to the facade interface

[Copilot]

The EMPTY_DATA constant is no longer referenced after the refactoring. Consider removing it to clean up unused code.

Suggested change

private static readonly Data EMPTY_DATA = new Data { Binary = EMPTY_BINARY };

[Copilot]

After updating the body section, reset any cached readers/writers (byteIn, byteOut) to avoid stale stream state when consuming or writing content again.

Suggested change

	set => Message.BodySection = new Data { Binary = value };
	set
	{
	Message.BodySection = new Data { Binary = value };
	byteIn = null;
	byteOut = null;
	}

[Copilot]

[nitpick] This private overload shares the same name as the public ReadBytes method, which may cause confusion. Consider renaming it to something like ReadBytesInternal.

Suggested change

	return ReadBytes(dataIn, value, length);
	}
	private int ReadBytes(BinaryReader binaryReader, byte[] value, int length)
	return ReadBytesInternal(dataIn, value, length);
	}
	private int ReadBytesInternal(BinaryReader binaryReader, byte[] value, int length)