Skip to content

feat(specs): update try transformation specs for no-code #4974

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 12, 2025
Merged

feat(specs): update try transformation specs for no-code #4974

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
437cf0c
feat(specs): update try transformation specs for no-code
mehmetaligok Jun 11, 2025
fa35a7b
fix(specs): remove required fields to not break existing integrations
mehmetaligok Jun 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion specs/ingestion/common/schemas/transformation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ TransformationTry:
properties:
code:
$ref: '#/Code'
type:
$ref: '#/TransformationType'
input:
$ref: '#/TransformationInput'
sampleRecord:
description: The record to apply the given code to.
type: object
Expand All @@ -161,7 +165,8 @@ TransformationTry:
items:
$ref: './authentication.yml#/AuthenticationCreate'
required:
- code
- type
- input
Copy link
Collaborator

@Fluf22 Fluf22 Jun 12, 2025

Choose a reason for hiding this comment

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

I don't think we should require them here
We could create another object where it's required, but here, it would be a bc I think

Copy link
Contributor Author

@mehmetaligok mehmetaligok Jun 12, 2025

Choose a reason for hiding this comment

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

Sorry, I didn't get it.

The code field was previously required; now, instead of the code field, we want to use type and input. So that is why I made them required, because the endpoint wouldn't work without them.

Copy link
Collaborator

@Fluf22 Fluf22 Jun 12, 2025

Choose a reason for hiding this comment

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

Yes, but if people use this prop in their code, it will not work anymore.
For example, if you have this in your JS code:

const transformation: Transformation = { code: '' }

Typescript will be yelling at you because you don't implement type and input, when you'll upgrade to the latest version

Copy link
Contributor

@millotp millotp Jun 12, 2025

Choose a reason for hiding this comment

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

it still works with the legacy payload right ? @Fluf22 is right, this is a breaking change if you put them as required

Copy link
Contributor

@millotp millotp Jun 12, 2025

Choose a reason for hiding this comment

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

I sent my message without seeing your second one thomas

Copy link
Contributor Author

@mehmetaligok mehmetaligok Jun 12, 2025

Choose a reason for hiding this comment

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

Okay. I didnt aware of this will effect and break the generated code. Btw, this is also the case for create payload so I need to revert that change.

But what is the recommendation in here? Make nothing required?

(The api supports legacy payload and the new payload at the same time. So there is no breaking change in the actual API.)

Copy link
Collaborator

@Fluf22 Fluf22 Jun 12, 2025

Choose a reason for hiding this comment

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

@millotp I'd suggest creating a second payload and using a oneOf?
But maybe this would be bc in some languages?

Copy link
Collaborator

@Fluf22 Fluf22 Jun 12, 2025

Choose a reason for hiding this comment

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

Only other option is to make nothing required, yes

Copy link
Contributor

@millotp millotp Jun 12, 2025

Choose a reason for hiding this comment

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

having a oneOf is breaking too, I would suggest making everything optional for now, and in a month or so, make the new payload required and leave code as optional.
We don't have a lot of way to deal with breaking changes because of the SLA...

Copy link
Contributor Author

@mehmetaligok mehmetaligok Jun 12, 2025

Choose a reason for hiding this comment

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

updated with fa35a7b

- sampleRecord

TransformationTryResponse:
Expand Down
12 changes: 8 additions & 4 deletions tests/CTS/requests/ingestion/tryTransformation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
[
{
"parameters": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
}
Expand All @@ -10,7 +11,8 @@
"path": "/1/transformations/try",
"method": "POST",
"body": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
}
Expand All @@ -20,7 +22,8 @@
{
"testName": "with authentications",
"parameters": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
},
Expand All @@ -40,7 +43,8 @@
"path": "/1/transformations/try",
"method": "POST",
"body": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
},
Expand Down
12 changes: 8 additions & 4 deletions tests/CTS/requests/ingestion/tryTransformationBeforeUpdate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"parameters": {
"transformationID": "6c02aeb1-775e-418e-870b-1faccd4b2c0f",
"transformationTry": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
}
Expand All @@ -13,7 +14,8 @@
"path": "/1/transformations/6c02aeb1-775e-418e-870b-1faccd4b2c0f/try",
"method": "POST",
"body": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
}
Expand All @@ -25,7 +27,8 @@
"parameters": {
"transformationID": "6c02aeb1-775e-418e-870b-1faccd4b2c0f",
"transformationTry": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
},
Expand All @@ -46,7 +49,8 @@
"path": "/1/transformations/6c02aeb1-775e-418e-870b-1faccd4b2c0f/try",
"method": "POST",
"body": {
"code": "foo",
"type": "code",
"input": {"code": "foo"},
"sampleRecord": {
"bar": "baz"
},
Expand Down