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

NullableAttribute not checked for in cache #325

Closed
jnystad opened this issue Aug 25, 2022 · 4 comments · Fixed by #327
Closed

NullableAttribute not checked for in cache #325

jnystad opened this issue Aug 25, 2022 · 4 comments · Fixed by #327
Labels
bug Something isn't working pkg:schema-generation

Comments

@jnystad
Copy link

jnystad commented Aug 25, 2022

Environment

  • Nuget Package: JsonSchema.Net.Generation
  • Nuget Version: 3.0.2
  • OS: Linux (WSL Ubuntu)
  • .Net Target: 6.0

Describe the bug

NullableAttribute by default has no IAttributeHandler (it is instead handled in NullabilityRefiner) and is therefore ignored in WhereHandled filter in SchemaGenerationContextCache.Get() used to calculate hash.

To Reproduce

Have several members of same type but only some with Nullable attribute. The non-nullable members may be "infected" with null.

Expected behavior

Nullable should be included in list of attributes passed to CalculateHash to ensure different hash.

Additional context

Possible fix is adding IAttributeHandler to NullableAttribute with an empty implementation.

@jnystad jnystad added the bug Something isn't working label Aug 25, 2022
@gregsdennis
Copy link
Owner

Interesting find. This may have crept in when I allowed [Nullable] to be applied to types instead of just members. I don't think I intended this attribute to be used on types, and this may be a mistake. I definitely don't have any tests for that use case.

Do you have the attribute on a type or just on members?

@jnystad
Copy link
Author

jnystad commented Aug 26, 2022

I have only tested it on members.

In general, this applies to any attribute that is only handled in refiners, and not attribute handlers. So perhaps there should be a way to indicate that attributes affects type info in some way. Otherwise, adding an empty IAttributeHandler for the offending attribute seems to be a manageable workaround for now.

Btw, if anyone tries this workaround: The IAttributeHandler implementation cannot be an internal class, as it will not be discovered via the Assembly (there might be more conditions due to this, I haven't tested several build configurations).

@gregsdennis
Copy link
Owner

gregsdennis commented Aug 30, 2022

Okay. I see what's happening now. (You're right, but I had to follow it in the code to see it.)

When I process a type, I also consider the attributes. However, I don't want to consider attributes that I don't care about (e.g. [JsonConverter]), so I have a .WhereHandled() extension method that filters out those attributes. The current implementation of .WhereHandled() is written to only care about attributes that implement IAttributeHandler or have a handler registered. Since neither of these apply to [Nullable], having a reference type with [Nullable] matches the same cache entry as that type without [Nullable].

This is what causes the contamination across properties and why the workaround fixes it.

I'll update [Nullable] to implement IAttributeHandler as a no-op so that it's just included by default.

@jnystad
Copy link
Author

jnystad commented Aug 30, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:schema-generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants