Improve Avalonia Support#562
Conversation
📝 WalkthroughWalkthroughEntityCollection, EntitySet, and LoadResult were extended to implement indexable collection interfaces (IList / IReadOnlyList), added indexers, refactored removal APIs to be index-aware (TryRemoveEntity/RemoveEntity(entity, index)), and adjusted collection-change notification behavior and related explicit interface implementations. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Coll as EntityCollection<T>
participant Source as SourceSet
participant Events as NotificationSystem
rect rgba(100,150,200,0.5)
Note over Client,Coll: Client removes an entity
Client->>Coll: Remove(entity)
Coll->>Coll: TryRemoveEntity(entity, out idx)
alt removed (idx >= 0)
Coll->>Events: Notify Remove at idx
else not present
Coll->>Events: (no notification)
end
end
rect rgba(150,100,200,0.5)
Note over Source,Coll: Source set collection changes
Source->>Coll: SourceSet_CollectionChanged(args)
alt items added
Coll->>Coll: materialize new items (ToList), compute newStartingIdx
Coll->>Events: Notify Add for range starting at newStartingIdx
else items removed
loop per removed entity
Coll->>Coll: TryRemoveEntity(entity, out idx)
alt removed (idx >= 0)
Coll->>Events: Notify Remove at idx for that entity
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OpenRiaServices.Client/Framework/EntityCollection.cs (1)
1018-1051: 🧹 Nitpick | 🔵 Trivial
IReadOnlyList<TEntity>indexer does not callLoad().Line 1019: The indexer accesses
Entities[index]directly without callingLoad(). If accessed beforeCount(which triggersLoad()), this could return incorrect results or throw. Consider callingLoad()for safety, or document thatCountmust be accessed first.♻️ Option to add Load() call for safety
`#region` ICollection<TEntity>, IReadOnlyList<TEntity> Members - TEntity IReadOnlyList<TEntity>.this[int index] => Entities[index]; + TEntity IReadOnlyList<TEntity>.this[int index] + { + get + { + Load(); + return Entities[index]; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1018 - 1051, The IReadOnlyList<TEntity> indexer getter currently returns Entities[index] directly (IReadOnlyList<TEntity>.this[int index] => Entities[index]); modify this getter to call Load() before accessing Entities to ensure the collection is initialized (i.e., call this.Load() then return Entities[index]); update any related tests or callers if necessary to account for potential lazy loading side-effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1116-1119: The IList.Remove implementation in EntityCollection
currently casts the incoming object directly and will throw InvalidCastException
for non-TEntity values; update the IList.Remove method to first check the type
(e.g., use "is TEntity" or "as TEntity") and only call Remove(TEntity) when the
value is a TEntity, otherwise do nothing (mirror the safe behavior of
IList.Contains) to avoid exceptions for wrong types.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 985-988: The NotSupportedException in IList.RemoveAt (in class
EntitySet) has a typo in its message ("RemoteAt"); update the format argument to
"RemoveAt" so the Resource.IsNotSupported message correctly references the
RemoveAt operation (i.e., change the string passed into the string.Format call
from "RemoteAt" to "RemoveAt" in the IList.RemoveAt method).
- Around line 1271-1272: The List property currently references itself causing
infinite recursion; change its getter to return the base class list cast to
List<TEntity> (i.e., use base.List) so the property reads the inherited
collection instead of calling itself; update the property declared as "private
new List<TEntity> List" to return (List<TEntity>)base.List to fix
StackOverflowException when accessing List in EntitySet or the List property
accessor.
- Around line 11-12: Remove the unused using directive "using
System.Diagnostics.Contracts;" from the top of the file; locate the EntitySet
class in src/OpenRiaServices.Client/Framework/EntitySet.cs and delete that using
line so only actually referenced namespaces remain, ensuring no other code in
EntitySet (constructors, methods, properties) depends on
System.Diagnostics.Contracts.
---
Outside diff comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1018-1051: The IReadOnlyList<TEntity> indexer getter currently
returns Entities[index] directly (IReadOnlyList<TEntity>.this[int index] =>
Entities[index]); modify this getter to call Load() before accessing Entities to
ensure the collection is initialized (i.e., call this.Load() then return
Entities[index]); update any related tests or callers if necessary to account
for potential lazy loading side-effects.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee3e524b-b64f-4539-83bf-d2f17b0ef8bf
📒 Files selected for processing (2)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Todo: look at LoadOperation and LoadResult |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Line 21: The IList capability flags on EntitySet are inconsistent with
ListCollectionViewProxy<T>; change the IList implementations on EntitySet so
IList.IsFixedSize and IList.IsReadOnly mirror the underlying add/remove
capabilities the same way the proxy does: compute both properties as
!(this.CanAdd || this.CanRemove) (or equivalent using the EntitySet's
CanAdd/CanRemove members) rather than hardcoding IsFixedSize=false and relying
on IsReadOnly fallback; update the IList explicit interface implementations in
EntitySet to return that computed value so direct IList callers see the same
behavior as ListCollectionViewProxy<T>.
- Around line 955-960: IList.Add(object) currently assumes only one item is
added and returns -1 when Add(Entity) (which may call AddAttachInferrer.Infer())
expands the set by more than one; change the implementation so if Count !=
countBefore + 1 it returns the actual index of the inserted entity by calling
IndexOf((Entity)value) (or similar) instead of -1, otherwise keep returning
countBefore; update the IList.Add(object) method to use IndexOf when infer-add
expanded the collection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38e05d74-91e8-4043-b060-6c969c8a7281
📒 Files selected for processing (1)
src/OpenRiaServices.Client/Framework/EntitySet.cs
| /// Represents a collection of <see cref="Entity"/> instances. | ||
| /// </summary> | ||
| public abstract class EntitySet : IEnumerable, ICollection, INotifyCollectionChanged, IRevertibleChangeTracking, INotifyPropertyChanged | ||
| public abstract class EntitySet : IEnumerable, ICollection, IList, INotifyCollectionChanged, IRevertibleChangeTracking, INotifyPropertyChanged |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the EntitySet.cs file around line 947
head -n 960 src/OpenRiaServices.Client/Framework/EntitySet.cs | tail -n 30Repository: OpenRIAServices/OpenRiaServices
Length of output: 992
🏁 Script executed:
# Also check the class definition at line 21
head -n 30 src/OpenRiaServices.Client/Framework/EntitySet.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1153
🏁 Script executed:
# Search for ListCollectionViewProxy<T> definition
rg -n "class ListCollectionViewProxy" src/OpenRiaServices.Client/Framework/EntitySet.cs -A 20Repository: OpenRIAServices/OpenRiaServices
Length of output: 1035
🏁 Script executed:
# Find IsFixedSize and IsReadOnly implementations in the file
rg -n "IsFixedSize|IsReadOnly" src/OpenRiaServices.Client/Framework/EntitySet.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 235
🏁 Script executed:
# Check CanAdd and CanRemove properties
rg -n "CanAdd|CanRemove" src/OpenRiaServices.Client/Framework/EntitySet.cs | head -20Repository: OpenRIAServices/OpenRiaServices
Length of output: 464
🏁 Script executed:
# Check the EntitySet.IsReadOnly property definition
sed -n '150,170p' src/OpenRiaServices.Client/Framework/EntitySet.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 623
🏁 Script executed:
# Also verify IList.Add method to understand the return value semantics
sed -n '960,980p' src/OpenRiaServices.Client/Framework/EntitySet.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 651
🏁 Script executed:
# Let's verify the definitions of CanAdd and CanRemove to understand their semantics
sed -n '68,100p' src/OpenRiaServices.Client/Framework/EntitySet.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 991
🏁 Script executed:
# Also check EntitySetOperations enum to understand the relationship with _supportedOperations
rg -n "enum EntitySetOperations" src/OpenRiaServices.Client/Framework/ -A 10Repository: OpenRIAServices/OpenRiaServices
Length of output: 1055
Align direct IList capability flags with proxy behavior to prevent misleading capability advertisement.
Line 947 hardcodes IList.IsFixedSize to false, while ListCollectionViewProxy<T> (lines 1508, 1513) correctly returns !(this.Source.CanAdd || this.Source.CanRemove) for both properties. When an EntitySet is read-only (CanAdd and CanRemove both false), direct IList callers see IsFixedSize=false, contradicting the proxy's IsFixedSize=true. Additionally, no explicit IList.IsReadOnly exists, so it falls back to EntitySet.IsReadOnly—correct by coincidence but inconsistent with the proxy pattern. Apply the same formula to both properties:
Proposed fix
`#region` IList
- bool IList.IsFixedSize => false;
+ bool IList.IsFixedSize => !(this.CanAdd || this.CanRemove);
+ bool IList.IsReadOnly => !(this.CanAdd || this.CanRemove);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` at line 21, The IList
capability flags on EntitySet are inconsistent with ListCollectionViewProxy<T>;
change the IList implementations on EntitySet so IList.IsFixedSize and
IList.IsReadOnly mirror the underlying add/remove capabilities the same way the
proxy does: compute both properties as !(this.CanAdd || this.CanRemove) (or
equivalent using the EntitySet's CanAdd/CanRemove members) rather than
hardcoding IsFixedSize=false and relying on IsReadOnly fallback; update the
IList explicit interface implementations in EntitySet to return that computed
value so direct IList callers see the same behavior as
ListCollectionViewProxy<T>.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "IsFixedSize|IsReadOnly" src/OpenRiaServices.Client/Framework/EntitySet.cs
Length of output: 346
@Daniel-Svensson, thanks for the update! The fix looks good — the proxy (lines 1490/1492) now correctly delegates to ((IList)Source).IsFixedSize and ((IList)Source).IsReadOnly, making direct and proxy access consistent.
One minor note: IList.IsFixedSize is still hardcoded to false (line 947) even for fully read-only sets, whereas the correctly-computed IList.IsReadOnly (!(CanAdd || CanRemove)) would return true for them. Strictly speaking, a set with neither Add nor Remove should also report IsFixedSize = true. However, per the IList contract, callers are still expected to handle NotSupportedException, so this is functionally acceptable. Feel free to tighten it later if needed.
🧠 Learnings used
Learnt from: Daniel-Svensson
Repo: OpenRIAServices/OpenRiaServices PR: 512
File: src/OpenRiaServices.Server/Test/DomainServiceDescriptionTest.cs:385-387
Timestamp: 2026-03-08T23:08:53.507Z
Learning: In OpenRiaServices code reviews, ensure that the AssociationTypeDescriptor compatibility wrapper in DomainTypeDescriptionProvider.GetTypeDescriptor is applied only when there is a custom TypeDescriptionProvider and it does NOT originate from an OpenRiaServices assembly with version 5.7.0 or later. For OpenRiaServices >= 5.7.0, entities already emit EntityAssociationAttribute and must not be double-wrapped. This guideline applies to C# source files under src (e.g., any DomainTypeDescriptionProvider usage checks, tests, or related providers).
do some cleanup on ListCollectionViewProxy
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/OpenRiaServices.Client/Framework/EntitySet.cs (1)
946-950:⚠️ Potential issue | 🟠 MajorAdvertise fixed-size correctly on read-only sets.
When neither add nor remove is supported,
IList.IsReadOnlybecomestruebutIList.IsFixedSizestaysfalse.ListCollectionViewProxy<T>now forwards these flags fromEntitySet, so view code still sees immutable sets as resizable. Mirror the same capability check in both properties.♻️ Proposed fix
-bool IList.IsFixedSize => false; - -bool IList.IsReadOnly => (_supportedOperations & (EntitySetOperations.Remove | EntitySetOperations.Add)) == 0; +bool IList.IsFixedSize => !(this.CanAdd || this.CanRemove); + +bool IList.IsReadOnly => !(this.CanAdd || this.CanRemove);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` around lines 946 - 950, IList.IsFixedSize is incorrectly always false while IList.IsReadOnly correctly checks supported operations; make IsFixedSize mirror the same capability check as IsReadOnly by returning true when neither EntitySetOperations.Add nor EntitySetOperations.Remove is supported. Update the IList.IsFixedSize getter in EntitySet (and keep IList.IsReadOnly as-is) to use the same bitmask check against _supportedOperations (EntitySetOperations.Remove | EntitySetOperations.Add) so immutable sets advertise fixed size consistently for ListCollectionViewProxy<T> and other consumers.src/OpenRiaServices.Client/Framework/EntityCollection.cs (1)
1116-1119:⚠️ Potential issue | 🟠 MajorMake direct
IList.Removetolerant of probe calls.This still casts
objectdirectly and then delegates to the strictRemove(TEntity)path. That turns wrong-type or already-removed values into exceptions on the newIListsurface instead of a no-op.♻️ Proposed fix
void IList.Remove(object value) { - Remove((TEntity)value); + if (value is TEntity entity) + { + ((ICollection<TEntity>)this).Remove(entity); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1116 - 1119, The IList.Remove implementation should be tolerant of probe/wrong-type calls: instead of unconditionally casting object to TEntity, first check the runtime type and only call Remove(TEntity) when value is a TEntity (e.g., use "if (value is TEntity entity) Remove(entity);" or equivalent); otherwise return/no-op so wrong-type or already-removed/null probe calls do not throw on the IList surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1084-1090: IList.Add currently skips validation and returns
countBefore when it assumes exactly one item added; instead mirror
EntityCollection<TEntity>.ListCollectionViewProxy<T>.Add by validating/casting
the input (throwing the same ArgumentException/InvalidCast as Add(TEntity)
does), call the collection add logic (Add((TEntity)value) or underlying
Source.Add), then call IndexOf on the added entity to determine and return the
actual insertion index (or -1 if not found) so side-effects that add multiple
items still yield the correct index; use the same validation and IndexOf lookup
code paths as the proxy Add to keep behavior consistent.
- Around line 1018-1020: The public indexer TEntity this[int index] currently
returns Entities[index] without ensuring the collection is loaded; modify the
indexer to call Load() before accessing Entities to guarantee the cache is
populated (matching Count and GetEnumerator behavior) and update its comment to
document that Load() is invoked; apply the same change to the other public
indexer occurrences around lines referenced (e.g., the IList indexer already
documents expectation—mirror that behavior by invoking Load() in the public
indexer and any similar accessors such as the one at 1074-1082).
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 985-988: IList.Remove(object) currently casts value to Entity and
can throw InvalidCastException; change its implementation to mirror
Contains/IndexOf by first checking "value is Entity e" and only call Remove(e)
when true, otherwise do nothing (silently return) so incompatible types or null
don't cause exceptions—update the IList.Remove(object) method that currently
calls Remove((Entity)value) to use the safe type-check pattern.
---
Duplicate comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 1116-1119: The IList.Remove implementation should be tolerant of
probe/wrong-type calls: instead of unconditionally casting object to TEntity,
first check the runtime type and only call Remove(TEntity) when value is a
TEntity (e.g., use "if (value is TEntity entity) Remove(entity);" or
equivalent); otherwise return/no-op so wrong-type or already-removed/null probe
calls do not throw on the IList surface.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 946-950: IList.IsFixedSize is incorrectly always false while
IList.IsReadOnly correctly checks supported operations; make IsFixedSize mirror
the same capability check as IsReadOnly by returning true when neither
EntitySetOperations.Add nor EntitySetOperations.Remove is supported. Update the
IList.IsFixedSize getter in EntitySet (and keep IList.IsReadOnly as-is) to use
the same bitmask check against _supportedOperations (EntitySetOperations.Remove
| EntitySetOperations.Add) so immutable sets advertise fixed size consistently
for ListCollectionViewProxy<T> and other consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffcc4c47-7b9f-4bb7-9755-aaea82d1e515
📒 Files selected for processing (3)
src/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntitySet.cssrc/OpenRiaServices.Client/Framework/LoadResult.cs
| #region ICollection<TEntity>, IReadOnlyList<TEntity> Members | ||
| public TEntity this[int index] => Entities[index]; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "EntityCollection.cs" -type f 2>/dev/null | head -5Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
git ls-files | grep -i "entitycollection.cs"Repository: OpenRIAServices/OpenRiaServices
Length of output: 309
🏁 Script executed:
wc -l src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 136
🏁 Script executed:
sed -n '1010,1030p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 651
🏁 Script executed:
sed -n '1070,1090p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 782
🏁 Script executed:
# Search for Entities property definition
rg -n "public.*Entities.*{" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -10Repository: OpenRIAServices/OpenRiaServices
Length of output: 176
🏁 Script executed:
# Search for Load method definition
rg -n "public.*Load\(\)" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Search for Count property
rg -n "public.*Count.*{" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -5Repository: OpenRIAServices/OpenRiaServices
Length of output: 170
🏁 Script executed:
sed -n '1,1133p' src/OpenRiaServices.Client/Framework/EntityCollection.cs | grep -n "public.*Entities" | head -10Repository: OpenRIAServices/OpenRiaServices
Length of output: 138
🏁 Script executed:
grep -n "private.*_entities" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 253
🏁 Script executed:
grep -n "Entities.*{" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -15Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
sed -n '300,400p' src/OpenRiaServices.Client/Framework/EntityCollection.cs | cat -nRepository: OpenRIAServices/OpenRiaServices
Length of output: 4552
🏁 Script executed:
# Search for Load method
grep -n "void Load()" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 106
🏁 Script executed:
# Search for Count property
grep -n "public.*Count" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 136
🏁 Script executed:
sed -n '446,470p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1192
🏁 Script executed:
sed -n '176,200p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 721
🏁 Script executed:
# Search for Entities property around the _entities field
sed -n '100,200p' src/OpenRiaServices.Client/Framework/EntityCollection.cs | grep -A 10 "Entities"Repository: OpenRIAServices/OpenRiaServices
Length of output: 1187
🏁 Script executed:
# Let's look for the Entities property definition
rg -n "Entities\s*{" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 162
🏁 Script executed:
sed -n '100,250p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 5114
🏁 Script executed:
# Check if there are tests for the indexer behavior
find . -name "*EntityCollection*Test*" -o -name "*Test*EntityCollection*" | head -5Repository: OpenRIAServices/OpenRiaServices
Length of output: 447
🏁 Script executed:
# Look for other usages of the indexer in the codebase
rg '\[.*\]' src/OpenRiaServices.Client/Framework/EntityCollection.cs | grep -i "index\|entities" | head -20Repository: OpenRIAServices/OpenRiaServices
Length of output: 503
🏁 Script executed:
# Check if public methods other than Count call Load
grep -B 5 -A 10 "public.*this\[" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1247
🏁 Script executed:
# Check IEnumerator to see if it calls Load
sed -n '900,1000p' src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -50Repository: OpenRIAServices/OpenRiaServices
Length of output: 1969
🏁 Script executed:
# Get GetEnumerator implementation
grep -B 2 -A 8 "GetEnumerator" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -30Repository: OpenRIAServices/OpenRiaServices
Length of output: 1073
🏁 Script executed:
# Look at EntityCollectionTests to see if there are tests for indexer behavior
grep -n "this\[" src/OpenRiaServices.Client/Test/Client.Test/Data/EntityCollectionTests.cs | head -20Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Search for tests that access the indexer
grep -B 5 -A 5 "collection\[.*\]" src/OpenRiaServices.Client/Test/Client.Test/Data/EntityCollectionTests.cs | head -40Repository: OpenRIAServices/OpenRiaServices
Length of output: 426
🏁 Script executed:
# Check if Count is always called before index access in typical usage patterns
grep -B 10 -A 5 "IList.this" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1575
🏁 Script executed:
# Check git history to see if this comment about Load was added with the IList implementation
git log -p --all -S "We skip Load, since caller is expected" -- src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -100Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Check if there are any other public indexer accesses that might give context
rg -B 3 -A 3 "public.*IReadOnlyList" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 438
🏁 Script executed:
# Check if there are any performance implications or reasons why Load wasn't called in indexers
grep -B 20 "public sealed class EntityCollection" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -30Repository: OpenRIAServices/OpenRiaServices
Length of output: 770
🏁 Script executed:
# Look at how ListCollectionViewProxy uses the indexer to understand the pattern better
sed -n '900,980p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 2721
🏁 Script executed:
# Final check: see if there's any documentation about when Load should be called
grep -B 5 -A 15 "public sealed class EntityCollection" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1048
Ensure indexers load data before accessing the cached collection.
The public indexer at line 1019 reads Entities[index] directly without calling Load(). Since Entities creates an empty cache when the collection hasn't been loaded yet, accessing collection[index] can throw IndexOutOfRangeException or expose a partial view unless a prior call like Count or enumeration triggered the load.
The IList indexer explicitly documents this with a comment: "We skip Load, since caller is expected to have called .Count which triggers load." However, the public indexer has no such documentation, and callers may not realize the requirement. This inconsistency with Count and GetEnumerator(), both of which call Load() explicitly, creates a fragile API contract.
♻️ Proposed fix
-public TEntity this[int index] => Entities[index];
+public TEntity this[int index]
+{
+ get
+ {
+ this.Load();
+ return this.Entities[index];
+ }
+} object IList.this[int index]
{
get
{
- // We skip Load, since caller is expected to have called .Count which triggers load
- return Entities[index];
+ this.Load();
+ return this.Entities[index];
}
set => throw new NotSupportedException(string.Format(CultureInfo.InvariantCulture, Resource.IsNotSupported, "Index setter"));
}Also applies to: 1074-1082
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1018 -
1020, The public indexer TEntity this[int index] currently returns
Entities[index] without ensuring the collection is loaded; modify the indexer to
call Load() before accessing Entities to guarantee the cache is populated
(matching Count and GetEnumerator behavior) and update its comment to document
that Load() is invoked; apply the same change to the other public indexer
occurrences around lines referenced (e.g., the IList indexer already documents
expectation—mirror that behavior by invoking Load() in the public indexer and
any similar accessors such as the one at 1074-1082).
| int IList.Add(object value) | ||
| { | ||
| int countBefore = this.Count; | ||
| Add((TEntity)value); | ||
|
|
||
| return (this.Count == countBefore + 1) ? countBefore : -1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the proxy's Add method to compare logic
rg -A 20 "class ListCollectionViewProxy" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -50Repository: OpenRIAServices/OpenRiaServices
Length of output: 1166
🏁 Script executed:
# Search for the proxy's Add implementation
rg -B 5 -A 15 "ListCollectionViewProxy.*Add\(" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Search for the Add method in ListCollectionViewProxy more specifically
ast-grep --pattern 'class ListCollectionViewProxy {
$$$
Add($_) {
$$$
}
$$$
}'Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Look for validation in the proxy's Add method
rg -B 2 -A 20 "void Add\(" src/OpenRiaServices.Client/Framework/EntityCollection.cs | grep -A 20 "ListCollectionViewProxy" | head -40Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
# Check the entire file structure to understand the classes
rg "class.*Proxy|int IList.Add|void Add\(" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 295
🏁 Script executed:
# Get the full proxy's Add implementation
rg -B 5 -A 25 "public int Add\(object value\)" src/OpenRiaServices.Client/Framework/EntityCollection.cs | head -60Repository: OpenRIAServices/OpenRiaServices
Length of output: 1132
🏁 Script executed:
# Read the specific lines of IList.Add method
sed -n '1084,1090p' src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 277
🏁 Script executed:
# Look for validation near the proxy's Add method
rg -B 2 -A 30 "public int Add\(object value\)" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 1171
🏁 Script executed:
# Check if Resource.MustBeAnEntity exists
rg "MustBeAnEntity" src/OpenRiaServices.Client/Framework/Repository: OpenRIAServices/OpenRiaServices
Length of output: 584
🏁 Script executed:
# Find the EntityCollection.Add method implementation
rg -B 2 -A 10 "public void Add\(TEntity" src/OpenRiaServices.Client/Framework/EntityCollection.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 597
Mirror the proxy's Add logic on the direct IList path.
EntityCollection<TEntity>.ListCollectionViewProxy<T>.Add validates input and uses IndexOf to resolve the actual insertion index after Source.Add. The direct IList.Add lacks both: it skips validation entirely and assumes Count + 1, incorrectly returning -1 when side effects add multiple entities even though the requested entity was successfully inserted.
♻️ Proposed fix
int IList.Add(object value)
{
+ if (value is null)
+ {
+ throw new ArgumentNullException(nameof(value));
+ }
+ if (value is not TEntity entity)
+ {
+ throw new ArgumentException(
+ string.Format(CultureInfo.CurrentCulture, Resource.MustBeAnEntity, "value"),
+ nameof(value));
+ }
+
int countBefore = this.Count;
- Add((TEntity)value);
+ Add(entity);
- return (this.Count == countBefore + 1) ? countBefore : -1;
+ return this.Count > countBefore ? this.Entities.IndexOf(entity, countBefore) : -1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int IList.Add(object value) | |
| { | |
| int countBefore = this.Count; | |
| Add((TEntity)value); | |
| return (this.Count == countBefore + 1) ? countBefore : -1; | |
| } | |
| int IList.Add(object value) | |
| { | |
| if (value is null) | |
| { | |
| throw new ArgumentNullException(nameof(value)); | |
| } | |
| if (value is not TEntity entity) | |
| { | |
| throw new ArgumentException( | |
| string.Format(CultureInfo.CurrentCulture, Resource.MustBeAnEntity, "value"), | |
| nameof(value)); | |
| } | |
| int countBefore = this.Count; | |
| Add(entity); | |
| return this.Count > countBefore ? this.Entities.IndexOf(entity, countBefore) : -1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 1084 -
1090, IList.Add currently skips validation and returns countBefore when it
assumes exactly one item added; instead mirror
EntityCollection<TEntity>.ListCollectionViewProxy<T>.Add by validating/casting
the input (throwing the same ArgumentException/InvalidCast as Add(TEntity)
does), call the collection add logic (Add((TEntity)value) or underlying
Source.Add), then call IndexOf on the added entity to determine and return the
actual insertion index (or -1 if not found) so side-effects that add multiple
items still yield the correct index; use the same validation and IndexOf lookup
code paths as the proxy Add to keep behavior consistent.
| void IList.Remove(object value) | ||
| { | ||
| Remove((Entity)value); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 1000 src/OpenRiaServices.Client/Framework/EntitySet.cs | tail -n +975 | cat -nRepository: OpenRIAServices/OpenRiaServices
Length of output: 1016
🏁 Script executed:
rg "IList\.(Contains|IndexOf|Remove)" src/OpenRiaServices.Client/Framework/EntitySet.cs -A 5 -B 1Repository: OpenRIAServices/OpenRiaServices
Length of output: 650
🏁 Script executed:
rg "^\s*public\s+(?:virtual\s+)?(?:bool\s+)?Remove\s*\(" src/OpenRiaServices.Client/Framework/EntitySet.cs -A 10Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
find . -name "*Test*.cs" -o -name "*test*.cs" | head -20Repository: OpenRIAServices/OpenRiaServices
Length of output: 1669
🏁 Script executed:
rg "IList.*Remove|Remove.*object" src/OpenRiaServices.Client/Test/Client.Test/Data/EntitySetTests.cs -B 3 -A 10Repository: OpenRIAServices/OpenRiaServices
Length of output: 57
🏁 Script executed:
rg "Remove\s*\(\s*Entity\s+" src/OpenRiaServices.Client/Framework/EntitySet.cs -B 2 -A 8 | head -50Repository: OpenRIAServices/OpenRiaServices
Length of output: 445
Match IList.Remove(object) behavior with the rest of the list surface.
IList.Contains(object) safely checks value is Entity e && Contains(e), and IList.IndexOf(object) delegates to _list.IndexOf(value) which gracefully returns -1 for incompatible types. However, IList.Remove(object) directly casts to Entity without type checking, throwing InvalidCastException for wrong types and forcing binding callers to handle exceptions when probing with object values.
♻️ Proposed fix
void IList.Remove(object value)
{
- Remove((Entity)value);
+ if (value is Entity entity && this.Contains(entity))
+ {
+ Remove(entity);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void IList.Remove(object value) | |
| { | |
| Remove((Entity)value); | |
| } | |
| void IList.Remove(object value) | |
| { | |
| if (value is Entity entity && this.Contains(entity)) | |
| { | |
| Remove(entity); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` around lines 985 - 988,
IList.Remove(object) currently casts value to Entity and can throw
InvalidCastException; change its implementation to mirror Contains/IndexOf by
first checking "value is Entity e" and only call Remove(e) when true, otherwise
do nothing (silently return) so incompatible types or null don't cause
exceptions—update the IList.Remove(object) method that currently calls
Remove((Entity)value) to use the safe type-check pattern.
2 participants
When binding collections avalonia requires any collection that does implement INotifyCollectionChanged to also implement IList.
Contains(entity)publicSummary by CodeRabbit
New Features
Bug Fixes / Behavior