Skip to content

Enforce maximum length in entity instance ids - #756

Open
halspang wants to merge 2 commits into
microsoft:mainfrom
halspang:halspang/entity_instance_id_validation
Open

Enforce maximum length in entity instance ids#756
halspang wants to merge 2 commits into
microsoft:mainfrom
halspang:halspang/entity_instance_id_validation

Conversation

@halspang

Copy link
Copy Markdown
Member

Summary

What changed?

  • Enforce maximum length of entity instances

Why is this change needed?

  • The backend expects a maximum length of 100 characters for an Entity's ID. This change enforces that at the SDK level, allowing for an opt-in breaking change to eliminate bad behavior on the backend.

Issues / work items

  • Resolves #
  • Related #

Project checklist

  • Release notes are not required for the next release
    • Otherwise: Notes added to release_notes.md
  • Backport is not required
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • All required tests have been added/updated (unit tests, E2E tests)
  • Breaking change?
    • Impact: Users will no longer be able to specify Entity Instance Ids (combination of name and key) longer than 100 characters
    • Migration guidance: Use smaller name/key combos for Entities. If entities already exist with IDs that are too long, you must use the truncated value to access them: entityInstanceId.ToString().Take(100)

AI-assisted code disclosure (required)

Was an AI tool used? (select one)

  • No
  • Yes, AI helped write parts of this PR (e.g., GitHub Copilot)
  • Yes, an AI agent generated most of this PR

If AI was used:

  • Tool(s):
  • AI-assisted areas/files:
  • What you changed after AI output:

AI verification (required if AI was used):

  • I understand the code and can explain it
  • I verified referenced APIs/types exist and are correct
  • I reviewed edge cases/failure paths (timeouts, retries, cancellation, exceptions)
  • I reviewed concurrency/async behavior
  • I checked for unintended breaking or behavior changes

Testing

Automated tests

  • Result: Passed

Notes for reviewers

  • N/A

CopilotAI review requested due to automatic review settings July 2, 2026 19:55
@halspang
halspangforce-pushed the halspang/entity_instance_id_validation branch from 079be85 to 2a3b93dCompareJuly 2, 2026 19:56

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR enforces a maximum serialized length (100 chars) for EntityInstanceId values in the Abstractions layer, aligning SDK behavior with the backend’s entity instance ID constraints and preventing backend-side truncation/invalid IDs.

Changes:

  • Added a length check in EntityInstanceId construction to reject instance IDs whose string form exceeds 100 characters.
  • Added new unit tests covering basic EntityInstanceId behaviors and invalid inputs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

FileDescription
src/Abstractions/Entities/EntityInstanceId.csEnforces a 100-character maximum entity instance ID length during construction (also impacts FromString/JSON conversion).
test/Abstractions.Tests/Entities/EntityInstanceIdTests.csAdds unit tests for constructor behavior, normalization, parsing, and invalid formats/lengths.

Comment threadsrc/Abstractions/Entities/EntityInstanceId.cs Outdated
Comment threadsrc/Abstractions/Entities/EntityInstanceId.cs Outdated
Comment threadtest/Abstractions.Tests/Entities/EntityInstanceIdTests.cs
CopilotAI review requested due to automatic review settings July 2, 2026 19:58

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment threadCHANGELOG.md Outdated
Comment threadsrc/Abstractions/Entities/EntityInstanceId.cs
Comment threadtest/Abstractions.Tests/Entities/EntityInstanceIdTests.cs
CopilotAI review requested due to automatic review settings July 2, 2026 20:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment threadsrc/Abstractions/Entities/EntityInstanceId.cs
Comment threadCHANGELOG.md Outdated
Signed-off-by: Hal Spang <halspang@microsoft.com>
@halspang
halspangforce-pushed the halspang/entity_instance_id_validation branch from 72a2a06 to 4571f91CompareJuly 2, 2026 20:16
CopilotAI review requested due to automatic review settings July 30, 2026 20:30

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

test/Abstractions.Tests/Entities/EntityInstanceIdTests.cs:11

  • Test method names in this project typically follow the "MemberOrScenario_Condition_Expectation" style (e.g., Ctor_NullName_Default, FromString_Invalid_Throws...). Using the generic "Test..." prefix here is inconsistent and makes failures harder to scan in test output.
 public void TestValidEntityInstanceId()

src/Abstractions/Entities/EntityInstanceId.cs:37

  • The length validation uses MaxInstanceIdLength but the exception message hard-codes "100". This can drift if the limit is changed later and makes the constant less useful; interpolate the constant into the message.
 throw new ArgumentException($"entity instance ids may not exceed 100 characters in length, actual length {length}.");

test/Abstractions.Tests/Entities/EntityInstanceIdTests.cs:6

  • Namespace is inconsistent with the other entity tests in this project (e.g., other files under test/Abstractions.Tests/Entities use Microsoft.DurableTask.Entities.Tests). Aligning the namespace improves discoverability and keeps related tests grouped consistently.

This issue also appears on line 11 of the same file.

namespace Microsoft.DurableTask.Abstractions.Tests.Entities;

Sign up for freeto 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.

3 participants

@halspang@berndverst