Skip to content
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

[core-client] x-www-form-urlencoded parameters not serialized #14767

Closed
jeremymeng opened this issue Apr 7, 2021 · 9 comments
Closed

[core-client] x-www-form-urlencoded parameters not serialized #14767

jeremymeng opened this issue Apr 7, 2021 · 9 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.

Comments

@jeremymeng
Copy link
Member

jeremymeng commented Apr 7, 2021

Some ACR generated methods use application/x-www-form-urlencoded to send request. Currently an error is thrown when using those generated methods.

Here's new unit test that represents the scenario.

  it.only("should serialize x-www-form-urlencoded parameters", async () => {                                                                                                                                                                                                                                                                                        
    const httpRequest = createPipelineRequest({ url: "https://example.com" });                                                                                                                                                                                                                                                                                      
    const operationSpec: OperationSpec = {                                                                                                                                                                                                                                                                                                                          
      path: "/oauth2/exchange",                                                                                                                                                                                                                                                                                                                                     
      httpMethod: "POST",                                                                                                                                                                                                                                                                                                                                           
      responses: {                                                                                                                                                                                                                                                                                                                                                  
        200: {}                                                                                                                                                                                                                                                                                                                                                     
      },                                                                                                                                                                                                                                                                                                                                                            
      formDataParameters: [                                                                                                                                                                                                                                                                                                                                         
        {                                                                                                                                                                                                                                                                                                                                                           
          parameterPath: ["path1", "aadAccesstoken"],                                                                                                                                                                                                                                                                                                               
          mapper: {                                                                                                                                                                                                                                                                                                                                                 
            type: {                                                                                                                                                                                                                                                                                                                                                 
              name: "Composite",                                                                                                                                                                                                                                                                                                                                    
              modelProperties: {                                                                                                                                                                                                                                                                                                                                    
                grantType: {                                                                                                                                                                                                                                                                                                                                        
                  defaultValue: "access_token",                                                                                                                                                                                                                                                                                                                     
                  isConstant: true,                                                                                                                                                                                                                                                                                                                                 
                  serializedName: "grant_type",                                                                                                                                                                                                                                                                                                                     
                  type: {                                                                                                                                                                                                                                                                                                                                           
                    name: "String"                                                                                                                                                                                                                                                                                                                                  
                  }                                                                                                                                                                                                                                                                                                                                                 
                },                                                                                                                                                                                                                                                                                                                                                  
                service: {                                                                                                                                                                                                                                                                                                                                          
                  serializedName: "service",                                                                                                                                                                                                                                                                                                                        
                  required: true,                                                                                                                                                                                                                                                                                                                                   
                  type: {                                                                                                                                                                                                                                                                                                                                           
                    name: "String"                                                                                                                                                                                                                                                                                                                                  
                  }                                                                                                                                                                                                                                                                                                                                                 
                }                                                                                                                                                                                                                                                                                                                                                   
              }                                                                                                                                                                                                                                                                                                                                                     
            }                                                                                                                                                                                                                                                                                                                                                       
          }                                                                                                                                                                                                                                                                                                                                                         
        }                                                                                                                                                                                                                                                                                                                                                           
      ],                                                                                                                                                                                                                                                                                                                                                            
      headerParameters: [                                                                                                                                                                                                                                                                                                                                           
        {                                                                                                                                                                                                                                                                                                                                                           
          parameterPath: ["options", "contentType"],                                                                                                                                                                                                                                                                                                                
          mapper: {                                                                                                                                                                                                                                                                                                                                                 
            defaultValue: "application/x-www-form-urlencoded",                                                                                                                                                                                                                                                                                                      
            isConstant: true,                                                                                                                                                                                                                                                                                                                                       
            serializedName: "Content-Type",                                                                                                                                                                                                                                                                                                                         
            type: {                                                                                                                                                                                                                                                                                                                                                 
              name: "String"                                                                                                                                                                                                                                                                                                                                        
            }                                                                                                                                                                                                                                                                                                                                                       
          }                                                                                                                                                                                                                                                                                                                                                         
        }                                                                                                                                                                                                                                                                                                                                                           
      ],                                                                                                                                                                                                                                                                                                                                                            
      serializer: createSerializer()                                                                                                                                                                                                                                                                                                                                
    };                                                                                                                                                                                                                                                                                                                                                              
    serializeRequestBody(                                                                                                                                                                                                                                                                                                                                           
      httpRequest,                                                                                                                                                                                                                                                                                                                                                  
      {                                                                                                                                                                                                                                                                                                                                                             
        path1: {                                                                                                                                                                                                                                                                                                                                                    
          aadAccesstoken: {                                                                                                                                                                                                                                                                                                                                         
            grantType: "refresh_token",                                                                                                                                                                                                                                                                                                                             
            service: "myregistry.azurecr.io"                                                                                                                                                                                                                                                                                                                        
          }                                                                                                                                                                                                                                                                                                                                                         
        }                                                                                                                                                                                                                                                                                                                                                           
      },                                                                                                                                                                                                                                                                                                                                                            
      operationSpec                                                                                                                                                                                                                                                                                                                                                 
    );                                                                                                                                                                                                                                                                                                                                                              
    const response: PipelineResponse = {                                                                                                                                                                                                                                                                                                                            
      headers: createHttpHeaders({ location: "https://example.com/redirect" }),                                                                                                                                                                                                                                                                                     
      request: httpRequest,                                                                                                                                                                                                                                                                                                                                         
      status: 300                                                                                                                                                                                                                                                                                                                                                   
    };                                                                                                                                                                                                                                                                                                                                                              
    const policy = formDataPolicy();                                                                                                                                                                                                                                                                                                                                
    const next = (request: PipelineRequest) => {                                                                                                                                                                                                                                                                                                                    
      console.log(request.body);                                                                                                                                                                                                                                                                                                                                    
      assert.strictEqual(httpRequest.body, "grant_type=access_token&service=myregistry.azurecr.io");                                                                                                                                                                                                                                                                
      return Promise.resolve( response);                                                                                                                                                                                                                                                                                                                            
    }                                                                                                                                                                                                                                                                                                                                                               
    await policy.sendRequest(httpRequest, next);                                                                                                                                                                                                                                                                                                                    
  }); 
@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Apr 7, 2021
@jeremymeng jeremymeng added this to the [2021] May milestone Apr 7, 2021
@jeremymeng jeremymeng self-assigned this Apr 7, 2021
@jeremymeng jeremymeng modified the milestones: [2021] May, [2021] June May 12, 2021
@ramya-rao-a
Copy link
Contributor

What is the workaround that is being used at the moment?

@jeremymeng
Copy link
Member Author

What is the workaround that is being used at the moment?

// const acrRefreshToken = await this.authClient.authentication.exchangeAadTokenForAcrRefreshToken(
// {
// aadAccesstoken: {
// grantType: "access_token",
// service,
// aadAccesstoken: aadAccessToken,
// }
// }
// );
// TODO: (jeremymeng) revert custom sendOperationRequest call after FormData is working in core
const payload = `grant_type=access_token&service=${encodeURIComponent(
service
)}&access_token=${encodeURIComponent(aadAccessToken)}`;
const customOptions: CustomAuthOptions = {
payload
};
const acrRefreshToken = await this.authClient.sendOperationRequest<AcrRefreshToken>(
{ options: { ...options, ...customOptions } },
customExchangeAadTokenForAcrRefreshTokenOperationSpec
);

@jeremymeng
Copy link
Member Author

jeremymeng commented Sep 10, 2021

The generated ACR operation spec:

const exchangeAcrRefreshTokenForAcrAccessTokenOperationSpec: coreClient.OperationSpec = {
path: "/oauth2/token",
httpMethod: "POST",
responses: {
200: {
bodyMapper: Mappers.AcrAccessToken
},
default: {
bodyMapper: Mappers.AcrErrors
}
},
formDataParameters: [Parameters.acrRefreshToken],
urlParameters: [Parameters.url],
headerParameters: [Parameters.contentType3, Parameters.accept4],
serializer
};

In serialization policy

const formDataParameterValue = getOperationArgumentValueFromParameter(
we get

formDataParameterPropertyName of "path1.aadAccesstoken", and serialized value of { grant_type: 'access_token', service: 'myregistry.azurecr.io' }, so we are setting

  request.formData["path1.aadAccesstoken"] = { grant_type: 'access_token', service: 'myregistry.azurecr.io' }

while I'd expect form data to be

  request.formData["grant_type"] = "access_token";
  request.formData["service"] = "myregistry.azurecr.io";

@jeremymeng
Copy link
Member Author

@sarangan12 @joheredi do you know whether this is expected generated code? I looked at other languages but couldn't find similar things. They seems to just use form content (Python) or encoded content (.NET),

@sarangan12
Copy link
Member

@jeremymeng

Looking at the spec (provided in the first issue description), the parameterPath: ["path1", "aadAccesstoken"], then it has to be

 request.formData["path1.aadAccesstoken"] = { grant_type: 'access_token', service: 'myregistry.azurecr.io' }

I am not clear about the reasoning about the expectation:

request.formData["grant_type"] = "access_token";
request.formData["service"] = "myregistry.azurecr.io";

Could you please explain a little more?

@jeremymeng
Copy link
Member Author

I am not clear about the reasoning about the expectation:

This is what the service actually expects, which suggests that for form data, the parameter path path1.addAccesstToken doesn't matter.

For application/x-www-form-urlencoded content type, I'd expect a list of name-value string pairs, and they would be url encoded, each pair connected with "=" and all of them concatenated with "&" as delimitor. like "grant_type=access_token&service=myregistry.azurecr.io"

I wonder whether .NET or Python code gen have any special handling for form data parameters.

Right now one idea for fixing in core is to

  • check in formDataPolicy.ts that if a form data key-value pair has an object value, then flatten it and add each property's name-value pair.

but I am still trying to understand the problem.

@jeremymeng
Copy link
Member Author

kinda of related to #16007. It would be nice if we could generate parameters for form parameters for application/x-www-form-urlencoded content type too

@jeremymeng
Copy link
Member Author

Not sure what has changed in the codegen since this is logged. But now the generated code is different so this might not be an issue any more.

  formDataParameters: [
    Parameters.service,
    Parameters.scope,
    Parameters.refreshToken1,
    Parameters.grantType1
  ],

@jeremymeng
Copy link
Member Author

Fixed by #18560

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

3 participants