easternbloc / node-stomp-client

A STOMP client for Node.js
Other
93 stars 47 forks source link

Module fails if messages contain utf8 #3

Closed datafatmunger closed 10 years ago

datafatmunger commented 12 years ago

If a message contains utf8 StompFrameEmitter.prototype.parseBody function and the StompFrame.prototype.send function don't work properly. I managed to patch it (will fork) in a somewhat inelegant manor:

StompFrameEmitter.prototype.parseBody = function () {
  var bufferBuffer = new Buffer(this.buffer);

  if (this.frame.contentLength > -1) {
    var remainingLength = this.frame.contentLength - this.frame.body.length;

    this.frame.appendToBody(bufferBuffer.slice(0, remainingLength).toString());
    this.buffer = bufferBuffer.slice(remainingLength, bufferBuffer.length).toString();

    if (this.frame.contentLength == Buffer.byteLength(this.frame.body)) {
      this.frame.contentLength = -1;
    } else {
      return;
    }
  }

  var index = this.buffer.indexOf('\0');

  if (index == -1) {
    this.frame.appendToBody(this.buffer);
    this.buffer = '';
  } else {
    // The end of the frame has been identified, finish creating it
    this.frame.appendToBody(this.buffer.slice(0, index));

    var frameValidation = this.getFrameValidation(this.frame.command);

    if (frameValidation.isValid) {
      // Emit the frame and reset
      this.emit('frame', this.frame);             // Event emit to catch any frame emission
      this.emit(this.frame.command, this.frame);  // Specific frame emission
    } else {
      this.emit('parseError', { message: frameValidation.message });
    }

    this.frame = new StompFrame();
    this.incrementState();
    this.buffer = this.buffer.substr(index + 1);
  }
};

StompFrame.prototype.send = function(stream) {
  stream.write(this.command + '\n');
  for (var key in this.headers) {
    stream.write(key + ':' + this.headers[key] + '\n');
  }
  if (this.body.length > 0) {
    stream.write('content-length:' + Buffer.byteLength(this.body) + '\n');
  }
  stream.write('\n');
  if (this.body.length > 0) {
    stream.write(this.body);
  }
  stream.write('\0');
};
easternbloc commented 12 years ago

Could you write a failing test for this issue? Cheers

easternbloc commented 11 years ago

I've tried looking into this issue by writing a failing test but I can't see the issue. We use strings not buffers as STOMP is a Text Orientated Messaging Protocol. Can you explain why you want to use buffers?

datafatmunger commented 11 years ago

Hey Ben,

I need to use buffers because we often see messages like "één fiets" in Dutch (we are a Dutch startup). Therefore it is unsafe to assume 1 character = 8 bytes, and setting the content length of the body of a frame based on the string length often truncates the body. If you put the string in a buffer, and get the length of the buffer for the it will be the actual length of the body. I'm fairly confident in my fork, we've been using it in production for 6 months now.

easternbloc commented 11 years ago

Thanks for that. I'll see if I can get some tests written and take it from there. Time to whip out the unicode snowman ;)

sam-github commented 11 years ago

/to @bgraves I'm familiar with this problem in other languages, but I spent 10 minutes trying to create strings or buffers where the character length and byte length were different, and failed do to my lack of experience in node.js encodings. I can prepare a test for this against your changes if you provide some code snippets showing how to make buffers that trigger this problem. Thanks.

easternbloc commented 10 years ago

This is now fixed in 0.5.0