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

feat(sourcecode): Serialize URI string #900

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Mar 4, 2024

Context

Currently, only the component name gets serialized into the URI string, the remaining parameters ar serialized in the parameters dictionary as follows:

  - from:
      uri: timer
      parameters:
        period: "1000"
        timerName: template

While this works for the Camel CLI, CamelK doesn't support this schema at the moment, causing the deployment to fail.

This commit writes the path parameters into the URI string, in order to support CamelK deployments, f.i.:

  - from:
      uri: timer:template
      parameters:
        period: "1000"

fixes: #884

@lordrip lordrip force-pushed the feat/serialize-path-params branch 2 times, most recently from b2cee2b to 7429699 Compare March 4, 2024 17:07
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

searching how to reproduce but ended with a lot of double-dot:

image

@apupier
Copy link
Member

apupier commented Mar 5, 2024

extraDoubleDotAdded

@lordrip lordrip force-pushed the feat/serialize-path-params branch from 7429699 to 374dd63 Compare March 5, 2024 08:24
@lordrip lordrip requested a review from apupier March 5, 2024 08:33
@lordrip
Copy link
Member Author

lordrip commented Mar 5, 2024

Awesome, thanks @apupier for catching it, I pushed a fix for a related issue

@lordrip lordrip force-pushed the feat/serialize-path-params branch from 374dd63 to cbb3263 Compare March 5, 2024 08:37
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for rest-openapi, the path parameters are still written as attributes:

image

- route:
    id: route-4144
    from:
      id: from-1532
      uri: timer
      parameters:
        period: "1000"
        timerName: template
      steps:
        - log:
            id: log-3429
            message: template message
        - to:
            id: to-1027
            uri: rest-openapi
            parameters:
              operationId: myOperation
              specificationUri: afile-openapi.json

@apupier
Copy link
Member

apupier commented Mar 5, 2024

The component can be modified:
changingUriScheme

in source code, notice the resta instead of rest:

- route:
    id: route-4144
    from:
      id: from-1532
      uri: timer
      parameters:
        period: "1000"
        timerName: template
      steps:
        - log:
            id: log-3429
            message: template message
        - to:
            id: to-2923
            uri: resta
            parameters:
              uriTemplate: atemplate

@apupier
Copy link
Member

apupier commented Mar 5, 2024

surely related to previous one as it seems to be one-off shift but slightly different case:
FirstLetterStoredInPreviousPathParameter

@lordrip lordrip requested a review from apupier March 5, 2024 09:14
@lordrip lordrip force-pushed the feat/serialize-path-params branch 4 times, most recently from eeb767c to b7363c4 Compare March 6, 2024 08:42
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • reported issues fixed
  • not spotted new ones
  • seems all cases covered by automates test cases

@lordrip
Copy link
Member Author

lordrip commented Mar 6, 2024

  • reported issues fixed

    • not spotted new ones

    • seems all cases covered by automates test cases

Thanks for the @apupier, but now I think I spotted something else that I think it would be better to fix.

As an example, the rest component has the following syntax: rest:method:path:uriTemplate, and while method and path are required parameters, urlTemplate it is not.

At this moment, when serializing the URI, if the urlTemplate is not provided, we end up with the following URI:

uri: "rest:get:myPath:"

I think it's not ideal, as it would be preferable to ignore the last : when there's an optional parameter.

This will require a bit more love since we would need to check the upcoming parameters as well, considering that for multiple optional parameters, we could provide just the last one, in which case, we would need to serialize the entire URI up to the last parameter 🙀

@lordrip lordrip marked this pull request as draft March 6, 2024 11:44
Currently, only the component name gets serialized into the URI string,
the remaining parameters ar serialized in the parameters dictionary as
follows:

```yaml
  - from:
      uri: timer
      parameters:
        period: "1000"
        timerName: template
```

While this works for the Camel CLI, CamelK doesn't support this schema
at the moment, causing the deployment to fail.

This commit write the path parameters into the URI string, in order to
support CamelK deployments, f.i.:
```yaml
  - from:
      uri: timer:template
      parameters:
        period: "1000"
```

fixes: KaotoIO#884
@lordrip lordrip force-pushed the feat/serialize-path-params branch from b7363c4 to c42542e Compare March 6, 2024 13:22
@lordrip lordrip marked this pull request as ready for review March 6, 2024 13:23
@lordrip
Copy link
Member Author

lordrip commented Mar 6, 2024

Af this update, the optional parameters are only written if the next value is either required or provided.

@apupier apupier self-requested a review March 6, 2024 13:30
@lordrip lordrip merged commit 8c51244 into KaotoIO:main Mar 6, 2024
9 checks passed
@lordrip lordrip deleted the feat/serialize-path-params branch March 6, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write path parameters in the URI
2 participants