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.

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

Jump to bottom

Incorrect type check for Thing in SDK OrderedItemList #82

Closed
4 of 10 tasks
hpdekoning opened this issue Nov 25, 2019 · 2 comments · Fixed by #85
Closed
4 of 10 tasks

Incorrect type check for Thing in SDK OrderedItemList #82

hpdekoning opened this issue Nov 25, 2019 · 2 comments · Fixed by #85
Assignees
@IevhenIkonnykov
Labels
bug minor
Milestone
sprint 2

Comments

@hpdekoning
Copy link

hpdekoning commented Nov 25, 2019 •
edited
Loading

Prerequisites

  • I have written a descriptive issue title
  • I have verified that I am running the latest version of the CDP4-SDK
  • I have searched open and closed issues to ensure it has not already been reported

Description

At https://github.com/RHEAGROUP/CDP4-SDK-Community-Edition/blob/fce21a2b9bed488be13eac2533f301653a84a9ff/CDP4Common/Types/OrderedItemList.cs#L566

In OrderedItemList.Add(...) the comparison if (typeof(T) == typeof(Thing)) is incorrect because it only checks that the type of the incoming item is directly a Thing, not one of its subtypes.

The correct condition is if (item is Thing) which properly checks that the item is a Thing or any of its subtypes. Alternatively typeof(T).IsSubclassOf(typeof(Thing)) can be used, if you are sure T cannot be Thing but must be one of its subtypes.

Steps to Reproduce

None, found by code inspection.

System Configuration

Any code that uses the C# SDK.

  • CDP4 version:
  • CDP4Common: development branch
  • CDP4JsonSerializer:
  • CDP4Dal:
  • CDP4JsonFileDal:
  • CDP4ServicesDal:
  • CDP4WspDal:
  • Other:
  • Environment (Operating system, version and so on):
  • .NET Framework version:
  • Additional information: Please check for other possible occurrences of the same condition.
@samatstariongroup
Copy link
Member

samatstariongroup commented Nov 25, 2019 •
edited
Loading

While investigating the issue and reviewing the whole OrderedItemList class it might be a good idea to not only fix this issue, but clean-up and refactor the whole class. Some notes:

  • for the same kind of problematic/erroneous condition, different kinds of exceptions seem the thrown
  • some cleanup can be achieved by refactoring code into re-usable private methods
  • It would be best to not change the public API of the class, unless this is absolutely necessary

@alexatstariongroup alexatstariongroup added this to the sprint 2 milestone Dec 6, 2019
@IevhenIkonnykov IevhenIkonnykov self-assigned this Dec 8, 2019
@IevhenIkonnykov IevhenIkonnykov mentioned this issue Dec 8, 2019
Merged
4 tasks
@alexatstariongroup
Copy link
Member

@hpdekoning it would be good if you could verify that the issue is indeed resolved.

@samatstariongroup samatstariongroup moved this to Done in COMET-CE Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@IevhenIkonnykov IevhenIkonnykov

Labels
bug minor
Projects
COMET-CE
Archived in project
Milestone
sprint 2
Development

Successfully merging a pull request may close this issue.

Fix type comparison in OrderedItemList STARIONGROUP/COMET-SDK-Community-Edition
4 participants
@IevhenIkonnykov @samatstariongroup @alexatstariongroup @hpdekoning