microsoft / kiota-serialization-json-ruby

Kiota serialization provider implementation for Ruby and JSON
https://aka.ms/kiota/docs
MIT License
1 stars 4 forks source link

Request body is an empty hash #20

Open sled opened 10 months ago

sled commented 10 months ago

When constructing a serialized request body, the body is empty because of these lines here:

https://github.com/microsoft/kiota-abstractions-ruby/blob/v0.14.3/lib/microsoft_kiota_abstractions/request_information.rb#L94-L107

        writer  = request_adapter.get_serialization_writer_factory().get_serialization_writer(content_type)
        @headers.try_add(@@content_type_header, content_type)
        if values != nil && values.kind_of?(Array)
          writer.write_collection_of_object_values(nil, values)
        else
          writer.write_object_value(nil, values);
        end
        this.content = writer.get_serialized_content();

The calls to writer.write_collection_of_object_values and writer.write_object_value with a nil key will return a new writer object, thus this.content = writer.get_serialized_content(); will be empty because it doesn't get written to the original writer.

The new writer objects are constructed here:

https://github.com/microsoft/kiota-serialization-json-ruby/blob/v0.7.0/lib/microsoft_kiota_serialization_json/json_serialization_writer.rb#L162-L165

    def write_object_value(key, value)
      if value
        if !key
          temp = JsonSerializationWriter.new()
          value.serialize(temp)
          return temp
        end
        begin
          temp = JsonSerializationWriter.new()
          value.serialize(temp)
          @writer[key] = temp.writer
        rescue StandardError => e
          raise e.class, "no key or value included in write_boolean_value(key, value)" 
        end
      end
    end

and here:

https://github.com/microsoft/kiota-serialization-json-ruby/blob/v0.7.0/lib/microsoft_kiota_serialization_json/json_serialization_writer.rb#L149-L152

sled commented 10 months ago

Here's a monkey patch to make it work:

monkey_patch = Module.new do
  def set_content_from_parsable(request_adapter, content_type, values)
      writer = request_adapter.get_serialization_writer_factory().get_serialization_writer(content_type)
      @headers.try_add(self.class.class_variable_get(:@@content_type_header), content_type)
      if values != nil && values.kind_of?(Array)
        @content = writer.write_collection_of_object_values(nil, values).map(&:get_serialized_content).to_json
      else
        @content = writer.write_object_value(nil, values).get_serialized_content
      end
  end
end

MicrosoftKiotaAbstractions::RequestInformation.prepend(monkey_patch)
baywet commented 10 months ago

Hi @sled Thanks for reporting this. I'm not sure how we ended up in a situation where this implementation:

  1. instantiates a writer
  2. returns it Both are drifts from the other languages implementations.

Is this something you'd be willing to submit a pull request to correct?

sled commented 10 months ago

@baywet writing an object could be done by merging the fields (because the content of the writer is stored in a hash map), however writing an array of objects or primitives might be an issue, because how would this work:

writer.writer_number_value('id', 1234)
writer.write_collection_of_object_values(nil, array_of_objects)

Could you point me to another implementation in C# / Java as a reference?

baywet commented 10 months ago

sure, there it is. https://github.com/microsoft/kiota-java/blob/abd2209818c017c45acbe3c7a386d1f52cfba111/components/serialization/json/src/main/java/com/microsoft/kiota/serialization/JsonSerializationWriter.java#L245

You'll see things about additionalDataToMerge, that's for composed types serialization. But that's not supported yet in Ruby, so you can ignore that additional complexity.

sled commented 10 months ago

@baywet I had a look at the Java implementation and C# implementation, both implementations rely on an underlying Json Writer library (com.google.gson.stream.JsonWriter / Utf8JsonWriter) which do hold some state (writeStartArray / writeEndArray) when writing, e.g.:

https://github.com/microsoft/kiota-serialization-json-dotnet/blob/main/src/JsonSerializationWriter.cs#L280-L290

public void WriteCollectionOfObjectValues<T>(string? key, IEnumerable<T>? values) where T : IParsable
{
    if(values != null)
    { //empty array is meaningful
        if(!string.IsNullOrEmpty(key)) writer.WritePropertyName(key!);
        writer.WriteStartArray();
        foreach(var item in values)
            WriteObjectValue<T>(null, item);
        writer.WriteEndArray();
    }
}

I think the Python implementation is easier to port because it doesn't rely on another library, e.g. https://github.com/microsoft/kiota-serialization-json-python/blob/main/kiota_serialization_json/json_serialization_writer.py#L214-L233

    def write_collection_of_object_values(
        self, key: Optional[str], values: Optional[List[U]]
    ) -> None:
        """Writes the specified collection of model objects to the stream with an optional
        given key.
        Args:
            key (Optional[str]): The key to be used for the written value. May be null.
            values (Optional[List[U]]): The collection of model objects to be written.
        """
        if isinstance(values, list):
            obj_list = []
            for val in values:
                temp_writer = self._create_new_writer()
                temp_writer.write_object_value(None, val)
                obj_list.append(temp_writer.value)

            if key:
                self.writer[key] = obj_list
            else:
                self.value = obj_list

The Python implementation is similiar to the Ruby one, it creates a temporary writer to serialize the items in the collection. The difference is, that if I write anything without key, it replaces the writer's internal value with the written value instead of returning a new writer. This should fix the problem.

baywet commented 10 months ago

Thanks for going the extra mile. Yes I had forgotten we were using a library in Java as well. Another one that might be interesting to look at is the go implementation

With all that context in mind, would you be willing to submit a pull request to address the issue?

sled commented 9 months ago

@baywet I can submit a PR sometime this week

sled commented 9 months ago

@baywet in the Ruby version, the writer methods raise a StandardError (why not ArgumentError?) when both key and value is not true-ish (i.e. nil or false). This doesn't seem to be the case in other languages like Java or Python.

Let's take the write_boolean_value method as an example:

Java: https://github.com/microsoft/kiota-java/blob/main/components/serialization/json/src/main/java/com/microsoft/kiota/serialization/JsonSerializationWriter.java#L54-L64

Python: https://github.com/microsoft/kiota-serialization-json-python/blob/main/kiota_serialization_json/json_serialization_writer.py#L42-L52

Go: https://github.com/microsoft/kiota-serialization-json-go/blob/main/json_serialization_writer.go#L98-L109

None of these raise an error if both key and value is nil / null / empty. They just do nothing.

Here's the Ruby implementation:

https://github.com/microsoft/kiota-serialization-json-ruby/blob/main/lib/microsoft_kiota_serialization_json/json_serialization_writer.rb#L34-L42

    def write_boolean_value(key, value)
      if !key && !value
        raise StandardError, "no key or value included in write_boolean_value(key, value)"
      end
      if !key 
        return value
      end
      @writer[key] = value
    end

This implementation raises an error if your try to write false without a key, which is a legitimate use-case, i.e.

writer.write_boolean_value(nil, false) # => raises StandardError
writer.write_boolean_value(nil, nil) # => raises StandardError

Should I adjust the behaviour to match the other implementations? i.e.

    def write_boolean_value(key, value)
      return if value.nil?

      if key.nil?
        @value = value
      else
        @writer[key] = value
      end
    end
sled commented 9 months ago

Also this is weird:

https://github.com/microsoft/kiota-serialization-json-ruby/blob/main/lib/microsoft_kiota_serialization_json/json_serialization_writer.rb#L214-L217

    def write_any_value(key, value)
        # ... 
        elsif value.is_a? Object
          return value.to_s
        else
          raise StandardError, "encountered unknown value type during serialization #{value.to_s}"
        end
    end

In Ruby everything is an object, the else clause raising an error is unreachable code.

baywet commented 9 months ago

Thanks for taking the time to look into this. Yes please align the implementation with other languages as much as possible. As for the object test, I don't know enough about the specifics of Ruby. From my limited knowledge on the topic it seems that every type is an object, but I wonder whether this was implemented due to an edge case?

itay-grudev commented 1 week ago

Can you please prioritize this. The implementations is obviously incorrect and is preventing POST requests with the Microsoft Graph Ruby SDK.

baywet commented 1 week ago

I want to make it clear that the Ruby SDK is currently not funded. That means he doesn't have any dedicated engineers allocated to it. This means we don't have anybody actively looking in this on the Microsoft side at this point. Now we understand that some people might already be using the SDK or even Kiota to generate clients. This is why we still reply to issues, and we also are more than happy to review any pull requests coming our way.

itay-grudev commented 1 week ago

Thanks for the feedback.