-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
uri template in RLC #2814
base: main
Are you sure you want to change the base?
uri template in RLC #2814
Conversation
.get(); | ||
assert.strictEqual(result.status, "204"); | ||
}); | ||
|
||
it("should have explode: true array", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case failed with 400 issue and I think we build correct url but failed in server side: Azure/cadl-ranch#731.
assert.strictEqual(result.status, "204"); | ||
}); | ||
|
||
it("should have explode: true primitive", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case failed with same reason, I plan to not generate wrapper type for primitive because the explode or style information will have no effect on the final url. Then I think this will not be an issue.
@timovv I think you could decide in core side if we need to support primitive value type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not throw here (in that case we would just ignore explode
if we don't get an array), but I agree with your plan and think it would be better if we just didn't generate the wrapper in this case, since the combination of primitive and explode: true
doesn't really make sense.
assert.strictEqual(result.status, "204"); | ||
}); | ||
|
||
it("should have explode: false record", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case failed with 400 either. This is a case for non-exploded object. Similar to array I think we should support the same default behavior for object in core's side to prepare path like param=a,1,b,2. @timov how do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-thinked this a little and it would be problematic if we convert any object to format like a,1,b,2 in core e.g if this is a Date, toString would be date string but we may not know how to format as form style. maybe here we should always generate it explictly in codegen.
As interface Param { value: Record<string, string>; explode: false; style: "form" }.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly it feels strange to me that this is considered a valid case. I don't really understand why we would serialize an object in this way. Is there a reference (specification or something) that describes this particular behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I see it described here: https://swagger.io/docs/specification/v3_0/describing-parameters/. We can implement that, but like you say there might be a problem for objects where toString
actually provides a meaningful value like Date. Do you know if there are any existing cases where RLC generates a type like that for a query parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about, if the object has a custom toString
implementation, use that, otherwise expand it?
fixes #2793
fixes #2792
Possible values
In-scoped cases
Path allowReserved
Support to define allowReserved for path parameter but the style and explode support for path parameter is not in-scope of this pr. The default for allowReserved would be
Query expansion
Please note the allowReserved for query paremeter is not in scope of this pr. Except the form style we would also include the spaceDelimited and pipeDelimited mainly considering backward-compatibility. The default setting would be:
/users{?id}
/users?id=5
/users?id=3,4,5
/users?id=role,admin,firstName,Alex
/users{?id*}
/users?id=5
/users?id=3&id=4&id=5
/users?role=admin&firstName=Alex
/users?id=3|4|5
We also have support for case in OpenAPI2 which were removed in openapi3 and we will remove this support also and we could dicuss on how to support when we have a real case yet.
Header expansion
Any expansion for header parameters would not be covered in this pr. We haven't met any real cases yet and we could expand this idea to header also if any in future.
Detailed cases
Case 1: allowReserved: true + path
Case 2: explode:false array + query
Case 3: explode:false object + query
Case 4: explode:true array + query
Case 5: explode:true object + query
Case 6: explode:true primitive + query
Case 7: explode:false array + query + ssv
Case 8: explode:false array + query + pipe
Case 9: explode:false array + query + tsv[not supported -> fallback to string]
Case 10: explode:false array + query + tsv[not supported -> fallback to string]
References