@andyjdavis opened this Pull Request on May 9th 2021 Contributor

Description:

This is a start towards fixing https://github.com/matomo-org/matomo/issues/7051

The return value when requesting a single column has now changed.
old: {"value":3161}
new: {"nb_visits":315}

The way I have done that has had some reasonably significant impacts on both JSON and XML output so I am unsure if I'm on the right track.

The core problem is that Renderer::convertDataTableToArray() contains a check for an array of length one. If an array of length 1 is detected its key is discarded and a single value is returned instead of an array. Instead of this array

array(1) {
  ["nb_visits"]=>
  float(315)
}

you just get this instead float(315) meaning that the array key has been discarded. There are then places that check for the single value where there should be an array and wrap that value in an array with key 'value'.

It does seem preferable in my mind for Renderer::convertDataTableToArray() to just return an array regardless of length and let code closer to output generation decide if a single element array requires special handling for display purposes but I could well be off base.

The code as it currently is in this PR has had various effects other than those above.

1) XML output is affected too. For example if you change the requested format from JSON to XML (/index.php?module=API&method=API.get&idSite=1&period=day&date=yesterday&format=XML&token_auth=anonymous&columns=nb_visits) you no longer get <result>2526</result> but get this instead.

<result>
    <nb_visits>2526</nb_visits>
</result>

This is the same format returned when you request multiple columns.

2) I'm not sure if the effects on nested arrays is desirable or not. The below is phpunit test output. Quite a few DataTable tests will require updating if they are to pass against this code (I'm happy to update the tests if this is the behaviour we want)

'<?xml version="1.0" encoding="utf-8" ?>\n
 <results>\n
    <result parentArrayKey="idSite">\n
-       <result testKey="row1">14</result>\n
-       <result testKey="row2">15</result>\n
+       <result testKey="row1">\n
+           <nb_visits>14</nb_visits>\n
+       </result>\n
+       <result testKey="row2">\n
+           <nb_visits>15</nb_visits>\n
+       </result>\n
        <result testKey="row3" />\n
    </result>\n
 </results>'
-'{"idSite":{"row1":14,"row2":15,"row3":[]}}'
+'{"idSite":{"row1":{"nb_visits":14},"row2":{"nb_visits":15},"row3":[]}}'

Again, this is the same format that is already return for multi-column queries.

Options:
1) Decide we want the column name preserved everywhere meaning that single column queries will match the format of multi-column queries. Both json and xml outputs will change substantially to bring single column queries in line with multi-column queries and downstream consumers will need to be updated where they expect the single column specific output format.

2) Decide we want the old behaviour in some contexts and I add in code to detect single column arrays and make the output match what it did previously. Make the tests pass essentially with very few test updates.

3) Something else?

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on May 9th 2021 Member

Hi @andyjdavis thanks for creating the pull request, but unfortunately we can't include this in an upcoming release since it would break BC for the API (for a lot of users). The only time we can include a change like this is for a major release, like version 5.0.0, where it's expected users will have to make changes if they want to use it. We certainly appreciate the effort you put in, though, and will be happy to look at any other pull requests you make!

@andyjdavis commented on May 10th 2021 Contributor

Makes sense. I am happy to revisit this once 5.0.0 development gets under way. Is it a good idea to have this issue added to the 5.0.0 milestone to make it less likely to miss that window of opportunity? https://github.com/matomo-org/matomo/milestone/134

Closing this pull request.

@diosmosis commented on May 10th 2021 Member

@andyjdavis good idea :+1:

This Pull Request was closed on May 10th 2021
Powered by GitHub Issue Mirror