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

fix: duration json type parsing fix #412

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

Zylius
Copy link
Contributor

@Zylius Zylius commented Apr 3, 2024

What was changed

Added ability to parse DurationJsonType instance.

Why?

It no longer works, and if you need to pass it around to other services or save ActivityOptions somewhere, stuff breaks. I've made this as a check, so it should be BC, and messages using older format shouldn't fail during deployment.

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Zigmas Satkevičius seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@roxblnfk roxblnfk added the Bug Something isn't working label Apr 3, 2024
@Zylius Zylius force-pushed the fix_json_interval_marshaling branch from 760c327 to 9ef9e62 Compare April 3, 2024 19:26
@Zylius
Copy link
Contributor Author

Zylius commented Apr 4, 2024

Any ideas when this could be merged? 🥺

@roxblnfk
Copy link
Collaborator

roxblnfk commented Apr 4, 2024

Could you add a test case?
Or an instruction how to check the issue

@Zylius
Copy link
Contributor Author

Zylius commented Apr 5, 2024

I cannot offer you tests, as they're functional and integrated within our system, but this snippet is basically already a test, just needs an assertion.

Without the code updated this will throw InvalidArgumentException

$type = new \Temporal\Internal\Marshaller\Type\DurationJsonType(new class implements \Temporal\Internal\Marshaller\MarshallerInterface
{
    public function marshal(object $from): mixed
    {
        return null;
    }

    public function unmarshal(array $from, object $to): object
    {
        return new \stdClass();
    }
});

$type->parse($type->serialize(1), null);

@Zylius Zylius force-pushed the fix_json_interval_marshaling branch from 9ef9e62 to ddba232 Compare April 5, 2024 11:33
@Zylius Zylius force-pushed the fix_json_interval_marshaling branch from ddba232 to 1545a3b Compare April 5, 2024 11:33
@Zylius
Copy link
Contributor Author

Zylius commented Apr 5, 2024

Got a few free minutes, added a test :) without the fix it fails

@roxblnfk
Copy link
Collaborator

roxblnfk commented Apr 5, 2024

Do you need the ability to unmarshal ActivityOptions from the old JSON structure?
If yes, please provide a JSON payload example, and I will add it to the tests and include the necessary attributes to make it work.

@Zylius
Copy link
Contributor Author

Zylius commented Apr 5, 2024

You saying where are more changes? I think only intervals changed

Here's the JSON payload exmple from 2.7.*

{
    "TaskQueueName": "test_queue",
    "ScheduleToCloseTimeout": 0,
    "ScheduleToStartTimeout": 0,
    "StartToCloseTimeout": 5000000000,
    "HeartbeatTimeout": 0,
    "WaitForCancellation": false,
    "ActivityID": "",
    "RetryPolicy": {
        "initial_interval": 0,
        "backoff_coefficient": 1,
        "maximum_interval": 0,
        "maximum_attempts": 4,
        "non_retryable_error_types": []
    }
}

Here is 2.8

{
    "TaskQueueName": "test_queue",
    "ScheduleToCloseTimeout": 0,
    "ScheduleToStartTimeout": 0,
    "StartToCloseTimeout": 5000000000,
    "HeartbeatTimeout": 0,
    "WaitForCancellation": false,
    "ActivityID": "",
    "RetryPolicy": {
        "initial_interval": {
            "seconds": 0,
            "nanos": 0
        },
        "backoff_coefficient": 1,
        "maximum_interval": {
            "seconds": 0,
            "nanos": 0
        },
        "maximum_attempts": 4,
        "non_retryable_error_types": []
    }
}

@roxblnfk
Copy link
Collaborator

roxblnfk commented Apr 5, 2024

It should be nanoseconds, right?

        "initial_interval": 0,
        "maximum_interval": 0,

@roxblnfk roxblnfk force-pushed the fix_json_interval_marshaling branch from b652f13 to 005c126 Compare April 5, 2024 13:31
@roxblnfk roxblnfk added this to the 2.9.1 milestone Apr 5, 2024
@Zylius
Copy link
Contributor Author

Zylius commented Apr 5, 2024

It should be nanoseconds, right?

        "initial_interval": 0,
        "maximum_interval": 0,

Yes the old format is in nanoseconds

@roxblnfk roxblnfk modified the milestones: 2.9.1, 2.8.2 Apr 5, 2024
@roxblnfk roxblnfk merged commit 03c654c into temporalio:master Apr 5, 2024
46 checks passed
@roxblnfk
Copy link
Collaborator

roxblnfk commented Apr 5, 2024

It's indeed an unusual case that DTOs of internal configurations are stored in the database or transferred between services.
However, it seems that such usage needs to be considered moving forward 🙂
SDK 2.8.2 has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants