Uploaded image for project: 'Olingo'
  1. Olingo
  2. OLINGO-1550

Aggregations not working due to JSON/XML serializer implementations

    XMLWordPrintableJSON

Details

    Description

      The current implementation of the ODataJsonSerializer and ODataXmlSerializer classes contain (at least) two issues that make $apply=groupby(...)) aggregations unusable in Olingo.

       

      ISSUE 1: The fields to be output are not properly computed.

       

      The writeProperties(...) method at the serializer implementations contain this:

          final boolean all = ExpandSelectHelper.isAll(select);
          final Set<String> selected = all ? new HashSet<String>() :
                  ExpandSelectHelper.getSelectedPropertyNames(select.getSelectItems());
      

      This helper only checks the presence of the $select query option and builds the list of fields to be output from that parameter. So if $select is not present it sets all = true and considers that all fields in the entity should be output.

      But this is not correct in the case of an aggregation, because only the fields that take part in the aggregation should be output even if $select is not specified, as can be seen in section "3.10 Transformation groupby" of the spec for the OData Extension for Data Aggregation Version 4.0:

      GET ~/Sales?$apply=groupby((Customer/Country,Product/Name),
                                 aggregate(Amount with sum as Total))
      

      …results in_

      {
        "@odata.context": "$metadata#Sales(Customer(Country),Product(Name),Total)",
        "value": [
          { "@odata.id": null, "Customer": { "Country" : "Netherlands" },
            "Product": { "Name": "Paper" }, "Total": 3 },
          { "@odata.id": null, "Customer": { "Country": "Netherlands" },
            "Product": { "Name": "Sugar" }, "Total": 2},
          { "@odata.id": null, "Customer": { "Country: "USA" },
            "Product": { "Name": "Coffee" }, "Total": 12},
          { "@odata.id": null, "Customer": { "Country: "USA" },
            "Product": { "Name": "Paper" },"Total": 5},
          { "@odata.id": null, "Customer": { "Country: "USA" },
            "Product": { "Name": "Sugar" }, "Total": 2}
        ]
      }
      

      So it seems ExpandSelectHelper should be taking any requested aggregations into account when computing the fields to be output.

        

      ISSUE 2: Primary Key fields are always forced into output.

       

      The same writeProperties(...) methods of both serializer implementations invoke this just before actually starting the serialization of the fields.

          addKeyPropertiesToSelected(selected, type);
      

      So even if a $select query option had been added to the request, or the above "issue 1" was fixed and not all fields were being selected for output when an aggregation is requested, this would still force the entity's PK fields into output in every case.

      This seems to have been added as a part of the fix for OLINGO-1246.

      But when an aggregation is performed, fields not taking part of the aggregation will never have a value, which is why their output should not be attempted. Unless the PK fields are part of the aggregation, there will be no possible values available for these fields because the entity objects they belong to should have been aggregated.

      So when this happens, the serializer will obtain a null value for the PK fields that have been added to the output by addKeyPropertiesToSelected(selected, type) –and which shouldn't be there–, check that these PK fields are not nullable and throw an exception:

      SerializerException: Non-nullable property not present!
      

      Lastly, note that I understand that adding the PK fields to every response payload does not go against the spec because, as explained in OLINGO-1246, the spec allows additional fields to be added when $select is used…  but doing this adds to the overall weight of the payload –especially with complex PKs– and therefore in my opinion should not be a default behaviour.

      Attachments

        Issue Links

          Activity

            People

              Unassigned Unassigned
              dfernandez Daniel Fernández
              Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated: