feat: add attribute controller support for matrix responses by MohamedM216 · Pull Request #5471 · valhalla/valhalla

MohamedM216 · 2025-08-25T17:05:55Z

Issue

This PR extends AttributesController to enable control over matrix serializer attributes to avoid parameter cluttering in case new pieces of information for a matrix connection are added in the future as described here #5063.

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too
If you made changes to a translation file, update transifex too

Requirements / Relations

#5063

…troller to functions signatures

kevinkreiser · 2025-08-25T19:56:10Z

dont forget to make a similar docs segment in the matrix docs to explain the different fields

MohamedM216 · 2025-08-25T20:40:00Z

Ah, right. I completely forgot about that. I’ll make sure to add it.

MohamedM216 · 2025-08-26T04:02:20Z

@kevinkreiser I have updated the docs. Addressed here a43e76d

MohamedM216 · 2025-10-01T12:21:12Z

Hi @kevinkreiser , just following up on this PR. Whenever you have time to review would be great. Thanks!

chrstnbwnkl · 2025-10-01T15:37:01Z

src/baldr/attributes_controller.cc

+    {kMatrixConnectionDateTime, true},
+    {kMatrixConnectionTimeZoneOffset, true},
+    {kMatrixConnectionTimeZoneName, true},
+    {kMatrixConnectionShape, true},


the default for shape should probably be false

chrstnbwnkl · 2025-10-01T15:39:15Z

src/tyr/matrix_serializer.cc

+
      writer.set_precision(tyr::kDefaultPrecision);
-      if (matrix.shapes().size() && shape_format != no_shape) {
+      if (controller(kMatrixConnectionShape) && matrix.shapes().size() && shape_format != no_shape) {


...and then this should be

Suggested change

if (controller(kMatrixConnectionShape) && matrix.shapes().size() && shape_format != no_shape) {

if (matrix.shapes().size() && (controller(kMatrixConnectionShape) || shape_format != no_shape)) {

...and you need to add a similar check in the part that decides whether to reserve space for shapes in the proto response.

OK! From the changes you made, I understand that the idea is: if the user specifies any shape format other than no_shape, they probably want shapes in the response, even if they didn't explicitly enable the shape attribute in the controller. Right?

chrstnbwnkl · 2025-10-01T15:40:44Z

src/tyr/matrix_serializer.cc

-    writer.end_array();

-    if (!(options.shape_format() == no_shape ||
+    if (controller(kMatrixConnectionShape) &&


adjust the check here as well to accommodate the fact that kMatrixConnectionShape is false by default

chrstnbwnkl · 2025-10-01T15:42:28Z

test/matrix.cc

  TimeDistanceMatrix timedist_matrix;
  timedist_matrix.SourceToTarget(request, reader, mode_costing, sif::TravelMode::kDrive, 400000.0);
-  json_res = tyr::serializeMatrix(request);
+  json_res = tyr::serializeMatrix(request, controller);


can you add some tests either here or the gurka matrix one that checks the JSON response?

chrstnbwnkl

Actually, I just realized that this just filters the JSON response, but we should probably apply this filter on the proto level already? Right now for example, the shapes may be needlessly added to the proto response and then discarded by the filter when converting to JSON.

kinkard · 2025-10-01T16:45:23Z

Actually, I just realized that this just filters the JSON response, but we should probably apply this filter on the proto level already? Right now for example, the shapes may be needlessly added to the proto response and then discarded by the filter when converting to JSON.

Then this feature will work for PBF response format as well and I'm just not sure that it is something worth effort and contract (all PBF object fields are filled) change. Especially taken that PBF is already size efficient enough.

…ribute-controller

…ons accordingly

MohamedM216 · 2025-10-05T06:41:07Z

test/matrix.cc

+                                                    << response << "\n\n";
+}
+
+TEST(Matrix, shape_controller_explicit_include) {


This one fails. I think the reason behind this is this condition [see below]
The json response:

{"sources_to_targets":[[{"from_index":0,"to_index":0,"time":473,"distance":5.88,"begin_heading":168.6,"end_heading":216.5,"begin_lat":52.103943,"begin_lon":5.068089,"end_lat":52.106118,"end_lon":5.101512},{"from_index":0,"to_index":1,"time":393,"distance":4.309,"begin_heading":168.6,"end_heading":276.0,"begin_lat":52.103943,"begin_lon":5.068089,"end_lat":52.100476,"end_lon":5.087027}]],"sources":[{"lat":52.103943,"lon":5.068089}],"targets":[{"lat":52.106118,"lon":5.101512},{"lat":52.100476,"lon":5.087027}],"units":"kilometers","algorithm":"costmatrix"}

I would appreciate your guidance on this.

MohamedM216 · 2025-10-05T06:42:09Z

src/tyr/matrix_serializer.cc

+    if ((controller(kMatrixConnectionShape) || options.shape_format() != no_shape) &&
+        request.matrix().algorithm() == Matrix::CostMatrix) {


This condition.

MohamedM216 · 2025-10-20T17:27:39Z

Hi @chrstnbwnkl , just following up. Whenever you have time to review would be great.

MohamedM216 added 7 commits August 25, 2025 11:57

extend AttributesController to include matrix api attributes

efdad1d

extend matrix_action to use AttributesController and add AttributeCon…

7e26130

…troller to functions signatures

modify the matrix serializer to use the attribute controller

6f0511e

fix a broken test

1a91467

make the default for all matrix attributes: true

fa665f0

add tests

11a5b89

update changelog

951ffe8

update docs: matrix/api-reference

a43e76d

chrstnbwnkl reviewed Oct 1, 2025

View reviewed changes

chrstnbwnkl requested changes Oct 1, 2025

View reviewed changes

MohamedM216 added 4 commits October 5, 2025 06:18

Merge branch 'master' of github.com:valhalla/valhalla into matrix-att…

7d64397

…ribute-controller

set kMatrixConnectionShape to false by default and adjust the conditi…

a145955

…ons accordingly

update shape condition

a2762b1

test shape attribute in json response

18c7c01

MohamedM216 commented Oct 5, 2025

View reviewed changes

MohamedM216 requested a review from chrstnbwnkl October 5, 2025 10:39

	if (controller(kMatrixConnectionShape) && matrix.shapes().size() && shape_format != no_shape) {
	if (matrix.shapes().size() && (controller(kMatrixConnectionShape) \|\| shape_format != no_shape)) {

		if ((controller(kMatrixConnectionShape) \|\| options.shape_format() != no_shape) &&
		request.matrix().algorithm() == Matrix::CostMatrix) {

Conversation

MohamedM216 commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

kevinkreiser commented Aug 25, 2025

Uh oh!

MohamedM216 commented Aug 25, 2025

Uh oh!

MohamedM216 commented Aug 26, 2025

Uh oh!

MohamedM216 commented Oct 1, 2025

Uh oh!

chrstnbwnkl Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

MohamedM216 Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl left a comment

Choose a reason for hiding this comment

Uh oh!

kinkard commented Oct 1, 2025

Uh oh!

MohamedM216 Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MohamedM216 Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

MohamedM216 commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MohamedM216 commented Aug 25, 2025 •

edited

Loading

MohamedM216 Oct 5, 2025 •

edited

Loading