QueryMap encoding does not bracket collections of maps or other collections

[diegotori]

Steps to Reproduce

1. Create a service endpoint with a @QueryMap parameter and its CRUD annotation's useBrackets property set to true.

class MyBooksService extends ChopperService {
  //...
  
  @Get(path: "api/books", useBrackets: true)
  Future<Response<List<String>>> findBooks(@QueryMap Map<String, dynamic> query);
}

2. Pass in the following query map:

{
  'filters': {
    r'$or': [
      {
        'date': {
          r'$eq': '2020-01-01',
        }
      },
      {
        'date': {
          r'$eq': '2020-01-02',
        }
      }
    ],
    'author': {
      'name': {
        r'$eq': 'Kai doe',
      },
    }
  }
}

3. Observe the generated URL:

Expected results:

GET /api/books?filters[$or][][date][$eq]=2020-01-01&filters[$or][][date][$eq]=2020-01-02&filters[author][name][$eq]=Kai%20doe

Actual results:

GET /api/books?filters[$or][]={date: {$eq: 2020-01-01}}&filters[$or][]={date: {$eq: 2020-01-02}}&filters[author][name][$eq]=Kai doe

Looks like this line in utils.dart is the culprit, since it's not recursively checking whether the item in the collection is either a map or another collection.

If I modify both _mapToQuery and _iterableToQuery to the below, it ends up producing the expected result:

Iterable<_Pair<String, String>> _mapToQuery(
  Map<String, dynamic> map, {
  String? prefix,
  bool useBrackets = false,
  bool includeNullQueryVars = false,
}) {
  final Set<_Pair<String, String>> pairs = {};

  map.forEach((key, value) {
    String name = Uri.encodeQueryComponent(key);

    if (prefix != null) {
      name = useBrackets
          ? '$prefix${Uri.encodeQueryComponent('[')}$name${Uri.encodeQueryComponent(']')}'
          : '$prefix.$name';
    }

    if (value != null) {
      if (value is Iterable) {
        pairs.addAll(_iterableToQuery(
          useBrackets ? '$name${Uri.encodeQueryComponent('[]')}' : name,
          value,
          prefix: name,
          useBrackets: useBrackets,
          includeNullQueryVars: includeNullQueryVars,
        ));
      } else if (value is Map<String, dynamic>) {
        pairs.addAll(
          _mapToQuery(
            value,
            prefix: name,
            useBrackets: useBrackets,
            includeNullQueryVars: includeNullQueryVars,
          ),
        );
      } else {
        pairs.add(
          _Pair<String, String>(name, _normalizeValue(value)),
        );
      }
    } else {
      if (includeNullQueryVars) {
        pairs.add(_Pair<String, String>(name, ''));
      }
    }
  });

  return pairs;
}

Iterable<_Pair<String, String>> _iterableToQuery(
  String name,
  Iterable values, {
  String? prefix,
  bool includeNullQueryVars = false,
  bool useBrackets = false,
}) {
  final List<_Pair<String, String>> result = [];
  final nonEmptyValues =
      values.where((value) => value?.toString().isNotEmpty ?? false).toList();
  for (dynamic value in nonEmptyValues) {
    if (value == null && includeNullQueryVars) {
      result.add(_Pair<String, String>(name, ''));
    } else if (value is Iterable) {
      result.addAll(_iterableToQuery(
          useBrackets ? '$name${Uri.encodeQueryComponent('[]')}' : name, value,
          prefix: name, useBrackets: useBrackets));
    } else if (value is Map<String, dynamic>) {
      result.addAll(
        _mapToQuery(
          value,
          prefix: name,
          useBrackets: useBrackets,
          includeNullQueryVars: includeNullQueryVars,
        ),
      );
    } else {
      result.add(
        _Pair<String, String>(
          name,
          _normalizeValue(value),
          useBrackets: useBrackets,
        ),
      );
    }
  }
  return result;
}

I dunno if there's a better way to handle this edge case, but this was my effort thus far.

This is a breaking issue when attempting to use Chopper to communicate with a Strapi server, since it uses qs based query formats when requesting filtered data.

[diegotori]

@techouse see the above.

[techouse]

@diegotori that is some convoluted query 🙈 haha 😂 Looks like something that's ripe for a POST request's body not a GET's query 🫠

I'll take a look.

[diegotori]

@diegotori that is some convoluted query 🙈 haha 😂 Looks like something that's ripe for a POST request's body not a GET's query 🫠

I'll take a look.

Yeah, if it were that easy. However, my company uses Strapi to expose content, and this is how you filter data when making requests to it.

[techouse]

@diegotori do you mind testing #585

dependency_overrides:
  chopper:
    git:
      url: https://github.com/lejard-h/chopper
      ref: fix/issue-584
      path: chopper

I used an elegant solution which will probably work

Iterable<_Pair<String, String>> _iterableToQuery(
  String name,
  Iterable values, {
  bool useBrackets = false,
  bool includeNullQueryVars = false,
}) =>
    [
      for (final dynamic value in values)
        if (value?.toString().isNotEmpty ?? false)
          if (value is Iterable)
            ..._iterableToQuery(
              name,
              value,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else if (value is Map<String, dynamic>)
            ..._mapToQuery(
              value,
              prefix: name,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else
            _Pair(
              name,
              _normalizeValue(value),
              useBrackets: useBrackets,
            )
    ];

Check the test example which takes a map like

{
  'filters': {
    r'$or': [
      {
        'date': {
          r'$eq': '2020-01-01',
        }
      },
      {
        'date': {
          r'$eq': '2020-01-02',
        }
      }
    ],
    'author': {
      'name': {
        r'$eq': 'Kai doe',
      },
    }
  }
}

and produces

filters.$or.date.$eq=2020-01-01&filters.$or.date.$eq=2020-01-02&filters.author.name.$eq=Kai doe

and withBrackets=true produces

filters[$or][date][$eq]=2020-01-01&filters[$or][date][$eq]=2020-01-02&filters[author][name][$eq]=Kai doe

[diegotori]

@techouse Shouldn't it add a bracket as part of the collection being created?

In other words, shouldn't it do:

filters[$or][][date][$eq]=2020-01-01&filters[$or][][date][$eq]=2020-01-02&filters[author][name][$eq]=Kai doe

I'll take a look at the PR either tonight or tomorrow.

[techouse]

@techouse Shouldn't it add a bracket as part of the collection being created?

Oh, yeah, you're right. Looks like that's what qs also does.

I'll take a look at the PR either tonight or tomorrow.

Feel free to push more code to it :)

[diegotori]

@techouse Maybe it can do:

Iterable<_Pair<String, String>> _iterableToQuery(
  String name,
  Iterable values, {
  bool useBrackets = false,
  bool includeNullQueryVars = false,
}) =>
    [
      for (final dynamic value in values)
        if (value?.toString().isNotEmpty ?? false)
          if (value is Iterable)
            ..._iterableToQuery(
              useBrackets ? '$name[] : name,
              value,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else if (value is Map<String, dynamic>)
            ..._mapToQuery(
              value,
              prefix: name,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else
            _Pair(
              name,
              _normalizeValue(value),
              useBrackets: useBrackets,
            )
    ];

[diegotori]

@techouse Shouldn't it add a bracket as part of the collection being created?

Hmm, good question. I'm, not sure. Gotta check the RFC on this one. What does qs do?

qs, which Strapi uses on its server, parses either [] or indexed (e.g. [0], [2], etc...) brackets, which indicates a collection. Chopper handled cases where the collection had simple primitive values, and it added [] to them for each pair:

{
  'one' {
    'two': [1, 2, 3] 
  }
}

Which produced

filters[one][two][]=1&filters[one][two][]=2&filters[one][two][]=3

The problem is, because it never handled anything beyond primitives, there were no examples where it would try to inject something other than simple.

The case that needs to be solved is:

• Given a collection in a query map.
• When creating the query string on an iterable value with elements that are also collections/maps
• Then add [] on the name of the collection being flattened, instead of on each item.

[diegotori]

@techouse Dio does this correctly in terms of adding the correct brackets due to encoding a collection on a query map.

Perhaps that might give you a roadmap from which you can follow.

[techouse]

Yeah, looks like a more involved problem + solution. I'll see when I'll have time to tackle this. Hopefully over the Easter break.

[techouse]

@techouse Maybe it can do:

Iterable<_Pair<String, String>> _iterableToQuery(
  String name,
  Iterable values, {
  bool useBrackets = false,
  bool includeNullQueryVars = false,
}) =>
    [
      for (final dynamic value in values)
        if (value?.toString().isNotEmpty ?? false)
          if (value is Iterable)
            ..._iterableToQuery(
              useBrackets ? '$name[] : name,
              value,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else if (value is Map<String, dynamic>)
            ..._mapToQuery(
              value,
              prefix: name,
              useBrackets: useBrackets,
              includeNullQueryVars: includeNullQueryVars,
            )
          else
            _Pair(
              name,
              _normalizeValue(value),
              useBrackets: useBrackets,
            )
    ];

Nah, that won't work because at that stage it's already a single item.

[techouse]

@diegotori I started slowly porting qs to Dart. Needless to say that it's quite the ordeal as it's old school non-typed JavaScript 😭

[diegotori]

@techouse I think I may have fixed it. Long story short, we have to add bracket suffixes to all collection types (map and list) when encoding.

In other words, this ended up working and passing the existing tests:

Iterable<_Pair<String, String>> _iterableToQuery(
  String name,
  Iterable values, {
  bool useBrackets = false,
  bool includeNullQueryVars = false,
}) {
  final bracketedName =
      '$name${useBrackets ? Uri.encodeQueryComponent('[]') : ''}';
  return [
    for (final dynamic value in values)
      if (value?.toString().isNotEmpty ?? false)
        if (value is Iterable)
          ..._iterableToQuery(
            bracketedName,
            value,
            useBrackets: useBrackets,
            includeNullQueryVars: includeNullQueryVars,
          )
        else if (value is Map<String, dynamic>)
          ..._mapToQuery(
            value,
            prefix: bracketedName,
            useBrackets: useBrackets,
            includeNullQueryVars: includeNullQueryVars,
          )
        else
          _Pair(
            name,
            _normalizeValue(value),
            useBrackets: useBrackets,
          )
  ];
}

Given the following query map:

{
  'filters': {
    r'$or': [
      {
        'date': {
          r'$eq': '2020-01-01',
        }
      },
      {
        'date': {
          r'$eq': '2020-01-02',
        }
      }
    ],
    'author': {
      'name': {
        r'$eq': 'Kai doe',
      },
    }
  }
}

It ended up producing the following encoded value:

filters[$or][][date][$eq]=2020-01-01&filters[$or][][date][$eq]=2020-01-02&filters[author][name][$eq]=Kai doe

Hopefully, this might suffice. I might end up submitting a PR to this effect shortly.

[techouse]

Nice work, however, I have already finished porting the stringify part of qs. I'm currently wrapping up the parse part. If all goes well it should be finished over the Easter holidays and I'll release it as a separate package. Using that we should have a more robust way of building queries.

[diegotori]

@techouse in that case, you may want to retain the test cases that I wrote for it. That way, you can use those to verify that your qs implementation works as expected.

[techouse]

Nothing will go to waste, my friend. 💪

[techouse]

@diegotori yesterday I releaded my Dart port of qs to the public. I'll make a PR using it to fix this issue.

P.S.: Here's your test case being put to good use ✅

[techouse]

@diegotori would you mind playing around with #592 and reporting back any findings / bugs? 🐛