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

Breaking change in release 7.2.2 #8680

Closed
jkonecki opened this issue Oct 20, 2023 · 5 comments · Fixed by #8681
Closed

Breaking change in release 7.2.2 #8680

jkonecki opened this issue Oct 20, 2023 · 5 comments · Fixed by #8681

Comments

@jkonecki
Copy link
Contributor

I believe there is an unexpected breaking change introduced in release 7.2.2 through this PR:

#8596

Code that uses CollectionAgeLimitAttribute and sets one of the fields will fail to compile.

    [CollectionAgeLimit(Days = 30)]
    public class ConnectionSearchGrain { }

The above code will compile in Orleans 7.2.1, but in 7.2.2. will cause the following compile-time error:

Error CS0617 'Days' is not a valid named attribute argument. Named attribute arguments must be fields which are not readonly, static, or const, or read-write properties which are public and not static.

This is due to the change of property setters from set to init, which prohibits from using them in atribute declarations.

I suspect this in an unintented change, as it makes fields like AlwaysActive, Days, Hours, Minutes unusable.
I recommend changing properties from init back to set.

@ghost ghost added the Needs: triage 🔍 label Oct 20, 2023
@jkonecki
Copy link
Contributor Author

jkonecki commented Oct 20, 2023

@ReubenBond would you like me to prepare a PR with the fix?

@ReubenBond
Copy link
Member

ReubenBond commented Oct 20, 2023

You're right, this is unintended. I've got a PR ready: #8681

@jkonecki
Copy link
Contributor Author

jkonecki commented Oct 20, 2023

That's great!

On an unrelated note, while looking at the source code I've noticed the below:

        private TimeSpan CalculateValue()
        {
            var span = AlwaysActive
            ? TimeSpan.FromDays(short.MaxValue)
            : TimeSpan.FromDays(Days) + TimeSpan.FromHours(Hours) + TimeSpan.FromMinutes(Minutes);
            return span <= TimeSpan.Zero
                ? MinAgeLimit
                : span;
        }

Shouldn't the last line be:

            return span < MinAgeLimit
                ? MinAgeLimit
                : span;

I know that at the moment MinAgeLimit is set to one minute, but if it was changed in the future (or a Seconds property was introduced) than it will be possible to set the span that is shorter than MinAgeLimit

@ReubenBond
Copy link
Member

Great catch, I've updated the PR

@jkonecki
Copy link
Contributor Author

jkonecki commented Oct 20, 2023 via email

@ghost ghost added the Status: Fixed label Oct 25, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants