Support unencoded reserved characters in path segments

[OlgaMaciaszek]

Some time ago, we had support for MatrixVariables added to Spring Cloud OpenFeign, using this processor.

This implementation works ok while sending requests to Spring REST controllers that can handle incorrectly encoded ; and = characters for matrix variables. However, an issue has been reported here that causes problems for servers that will not handle incorrect encoding of matrix variable reserved characters.

Basically, since matrix variables can appear in any path segment, they are handled by us as path params, with values stored in indexToExpander and get fully pct-encoded by feign-core, including their special characters, so we get /server/matrixParams%3Baccount%3Da%3Bname%3D instead of /server/matrixParams;account=a;name=n.

It looks like an issue that can only be addressed within feign-core and not as part of our integration. I could work on a fix if that's fine with you. An idea that comes to mind is to introduce some kind of markers, say {{}} that would surround characters that should not be encoded within expanded path param chunks, but I'm also open to work on any other solution that the team might prefer. Let me know what you think about it.

[kdavisk6]

@OlgaMaciaszek I agree our handling of this continues to be poor. We intentionally removed the encoded overrides because it's usage was not documented well and didn't have any effect since we refactored most of the template handling. It's my opinion that to correct this issue, we need to be able to provide a way for user's to manage URI encoding in a way that is both consistent with RFC 6570 and clear about it's usage and restrictions.

I've been leaning on support all 4 levels of expressions as the solution. Doing so would allow for programmatic assignment of the expression level, as in your use case, and direct usage of the specs template layouts. I'm planning on starting this in earnest very soon, but I don't want to block your issue.

What I think we can do in the meantime is add support for path style expressions first and release that asap. That should open up support and then you can refactor your processor to instead of registering a custom expander, just manipulate the template, ensuring that any values annotated with @MatrixVariables are represented in the template as ; prefixed values. Thoughts?

[oliverlockwood]

@OlgaMaciaszek could you please respond to this? @nickcodefresh and I are both keen to get the issue that's affecting us resolved (as per the linked ticket.)

[OlgaMaciaszek]

@kdavisk6 , @oliverlockwood Sorry for not getting back to you earlier. Have taken some time off after the holidays.

What I think we can do in the meantime is add support for path style expressions first and release that asap.

@kdavisk6 if it's going to allow us to set the encoding not to include the ; characters, then it will probably work for us just fine. However, could you please elaborate a bit more on the planned "support for path style expressions"? How would that work?

[oliverlockwood]

ping @kdavisk6 ?

[nickcodefresh]

Any update on this @OlgaMaciaszek @kdavisk6 ?

[dpodder]

+1 - I'm trying to use OpenFeign for a REST client in a different project (not related in any way to Spring Cloud OpenFeign), but hit a brick wall trying to support matrix parameters.

Is there any active discussion on this topic outside of this ticket?

[oliverlockwood]

@kdavisk6 @OlgaMaciaszek @velo this has now been open for 4 months without real activity - is there any chance we could have some input, please?

I'm happy to do some coding and contribute if you're able to point me in the right general direction and your preferred approach; what I really don't want to end up doing is maintaining a fork of this project. We have been blocked on upgrading Spring-Cloud for some time as a result of this issue, and that now blocks us from upgrading Spring-Boot, so it's pretty critical to us now.

[kdavisk6]

All, thank you for your patience. I understand the issue and it's importance to you all. The root of the issue is that our URI template expression handling is very simple and limited. I have an idea where we can move expansion of custom expanders deeper into the parsing. This would require expanders to manage pct-encoding directly however, which may break existing integrations.

I'd like more time to think it through.

[OlgaMaciaszek]

Hi @kdavisk6, has there been any progress regarding this issue?

[kdavisk6]

This is my fault. I will pick this up this week.

[oliverlockwood]

@kdavisk6 is there anything @nickcodefresh or I could do to assist you / test / etc?

[kdavisk6]

@OlgaMaciaszek @oliverlockwood I've added #1537 to enable support for Path Style parameters. Expressions that start with semi-colons ; are treated as path style, a.k.a matrix style parameters.

Examples

{;who}             ;who=fred
{;half}            ;half=50%25
{;empty}           ;empty
{;list}            ;list=red;list=green;list=blue
{;map}            ;semi=%3B;dot=.;comma=%2C

You no longer need a custom expander for this type of expression, as this change exposes PathStyleExpression as an Expander for use.

For example, in MatrixVariableParameterProcessor:

@Override
public boolean processArgument(AnnotatedParameterContext context, Annotation annotation, Method method) {
	int parameterIndex = context.getParameterIndex();
	Class<?> parameterType = method.getParameterTypes()[parameterIndex];
	MethodMetadata data = context.getMethodMetadata();
	String name = ANNOTATION.cast(annotation).value();

	checkState(emptyToNull(name) != null, "MatrixVariable annotation was empty on param %s.",
			context.getParameterIndex());

	context.setParameterName(name);
        data.indexToExpander().put(parameterIndex, new PathStyleExpression(name, null);
	return true;
}

If this meets your needs, please let me know and we'll work on getting it released.