Fix don't serialize DbContext

[joelweiss]

PR for #100

[RehanSaeed]

Just one very quick question in the review. Apart from that, it all looks good to me. I'll merge and release soon after.

[RehanSaeed]

Can you explain this change a little? I think it's just making sure that no duplicates are added. Is that something that can happen?

Perhaps a unit test would also help explain and test it.

[krajek]

I agree with @RehanSaeed, I reviewed the code and since we iterate over properties name generated by reflection there is no possibility of duplicates.
I would suggest removing the code unless you can demonstrate in the unit test that there is a possibility to introduce duplicates here.

[joelweiss]

Yeah, I should have made this a separate PR...

There is a possibility of having the same property name twice if the property is hidden.

The first commit was this check and the test as well.

[RehanSaeed]

Good spot on this Joel! If this was a separate PR, I think we could have merged now but no worries!

I'm not sure if we should ignore the property if we find a duplicate. We should probably add something to the end of the name and add it, so we don't lost anything. Thoughts @krajek?

[krajek]

I had the same thoughts yesterday.
For sure, dropping one of the properties is not the way to go.

We already had similar issue with Type property name.

if (propertiesBag.ContainsProperty("Type"))
            {
                if (!propertiesBag.ContainsProperty("$Type"))
                {
                    propertiesBag.AddProperty("$Type", valueType.FullName);
                }
                else
                {
                    // If both "Type" and "$Type" are present
                    // we just give up appending exception type
                }
            }
            else
            {
                propertiesBag.AddProperty("Type", valueType.FullName);
            }

[RehanSaeed]

In theory, there could be more than one property like this, so we should append something random to the end of the property name.

[joelweiss]

If this was a separate PR, I think we could have merged now but no worries!

It would still have to wait on the separate PR 😜, I noticed it because of this

---

What do you guys think of prepending the Namespace.TypeName if there is a collision?

private class ReflectionPropertyInfo
{
    public string Name { get; set; }
    public Type DeclaringType { get; set; }
    public Func<object, object> Getter { get; set; }
}
...
var key = values.ContainsKey(property.Name) ? $"{property.DeclaringType.FullName}.{property.Name}" : property.Name;
values.Add(key, destructuredValue);

[RehanSaeed]

That sounds like a great idea.

[joelweiss]

Done.

[RehanSaeed]

I forgot to mention that we should also mention that this NuGet package exists in the ReadMe.md file.

[RehanSaeed]

As thanks, I've listed you as a contributor on the front page:

https://github.com/RehanSaeed/Serilog.Exceptions/blob/master/README.md

[joelweiss]

I forgot to mention that we should also mention that this NuGet package exists in the ReadMe.md file.

Done, I added a note about Serilog.Exceptions.SqlServer as well, feel free to change the documentation to your liking.

[RehanSaeed]

The build is failing. I think that is because The Microsoft.SourceLink.GitHub package is being duplicated from the project reference.

@krajek Are you happy? It looks good to me.

[RehanSaeed]

Merged. I'll fix up the boken build and release. Thanks for your contribution.