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

Allow the parsers to fill POCOs with unknown/incorrect data #3050

Open
Kasdejong opened this issue Feb 19, 2025 · 8 comments
Open

Allow the parsers to fill POCOs with unknown/incorrect data #3050

Kasdejong opened this issue Feb 19, 2025 · 8 comments
Assignees

Comments

@Kasdejong
Copy link
Member

Kasdejong commented Feb 19, 2025

This also requires handling erroneous data in such a way that the errors are accurate when fetching the incorrectly filled properties.

Quick list of errors that SHOULD be handled by base (not the parsers) for reference:

  • Unknown elements
  • Expected primitive, got object or vice versa
  • Expected single, got array or vice versa
  • choice where no choice allowed
  • unknown choice
  • unknown resource
@ewoutkramer ewoutkramer removed their assignment Feb 26, 2025
@Kasdejong
Copy link
Member Author

After some discussion:
Rule of thumb: the GETTERS throw the exceptions.

If the parsers detect an anomaly, they do NOT set the backing field. Instead, they will leave it null and create an overflow entry containing the data.

When calling a getter, we check for null on the property backing field. If the backing field is null and the overflow dict EXISTS (check lazy initialized backing field), we try to find ourself in the overflow. If successful, we infer the parser error from the data. (We could also create an error datatype for this, which would be a wrapper around what would otherwise be in the dictionary directly)

For primitives: we stuff the value as found in the JsonValue (as if it were overflow)

Choice where no choice allowed: We consider it an unknown element unless we find a performant way to implement this.

@ewoutkramer
Copy link
Member

The disadvantage of using a wrapper in the overflow is of course that we introduce one more layer of indirection, and this means we need to adjust the DeepCopy, IsExactly, Matches etc methods to also be able to work with overflow entries that are not Base, but some kind of wrapper. Andrzej suggested that this wrapper van be a Base subclass, but still, it's a special case. The charm of the solution Kas original proposed, without the wrappers, is its natural simplicity. We'd only need them to carry information that the parser knows, but the objectmodel does not have. If that is so, we might also use Annotations, so the parser can communicate this extra data (like positional info in the json/xml file).

@ewoutkramer
Copy link
Member

Discussing with Andrzej, we discovered that the most natural way for the parser to work is to a) check whether the property is known an correct -> use the generated IL setter. If not, use SetValue(). This means we need to change SetValue so it can no longer fail: it will either set the property OR create an entry in the overflow. The beauty is that the user can do exactly the same, making the POCO's even closer to a DTO/dictionary. SetValue will allow you to set anything, and we will be able to roundtrip anything. If the value exists in the POCO and has the correct value -> the properties will allow you quick access. Otherwise: dictionary access.

@ewoutkramer
Copy link
Member

This is the design discussion of #2908.

@ewoutkramer
Copy link
Member

ewoutkramer commented Feb 27, 2025

If the backing field is null and the overflow dict EXISTS (check lazy initialized backing field), we try to find ourself in the overflow.

Just realized that we have to make sure we will not initialize the backing field as a side-effect of doing a lookup, so calling
Copy/Exact/Match/ListElements/GetValue/TryGetValue/indexer etc should not initialize the backingfield to an empty list, which I think is what will happen now:

// first: somePoco has no overflow
somePoco.IsExactly(otherPoco);
// now: somePoco has an initialized (but empty) overflow

// this will now trigger slower lookups:
var x = somePoco.SomeElement;

@ewoutkramer
Copy link
Member

Summary of the design is here: https://github.com/FirelyTeam/firely-net-sdk/wiki/Attribute-validation-and-Overflow (in progress).

@Kasdejong
Copy link
Member Author

Small note: we should also make sure SetValue clears the dictionary entry if a matching entry exists.

@ewoutkramer
Copy link
Member

When executing this task, you will need functionality in the parsers to "make up" the best (dynamic)type in case of unknown data. There is some code like that in the NewPocoBuilder (since it sometimes needs to do that guesswork if ITypedElement did not have type info), but you will need to augment it with parser-specific hints: e.g. if a Json property has a literal value (not an object), you know it's a primitive, even if you don't have type info (NewPocoBuilder polled the .Value property for this). Or the name start with _name (can only be for primitives). etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants