fix(i): JSON object default value panic

[nasdf]

Relevant issue(s)

Resolves #3146

Description

This PR fixes an issue where JSON default values containing objects would cause a panic related to a map key hash. Moving the default value off of the FieldDefinition appears to fix the issue.

Tasks

• I made sure the code is well commented, particularly hard-to-understand areas.
• I made sure the repository-held documentation is changed accordingly.
• I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
• I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Updated integration tests

Specify the platform(s) on which this was tested:

• MacOS

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.14%. Comparing base (b268c0b) to head (b75cf79).

Files with missing lines	Patch %	Lines
internal/lens/fetcher.go	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3156      +/-   ##
===========================================
+ Coverage    77.98%   78.14%   +0.15%     
===========================================
  Files          354      354              
  Lines        34669    34674       +5     
===========================================
+ Hits         27036    27093      +57     
+ Misses        6003     5966      -37     
+ Partials      1630     1615      -15     

Flag	Coverage Δ
all-tests	78.14% <94.74%> (+0.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
client/definitions.go	76.24% <100.00%> (+0.60%)	⬆️
internal/db/fetcher/encoded_doc.go	73.63% <100.00%> (ø)
internal/db/fetcher/fetcher.go	78.55% <100.00%> (ø)
internal/db/fetcher/indexer.go	83.92% <100.00%> (ø)
internal/lens/fetcher.go	71.63% <75.00%> (+1.40%)	⬆️

... and 14 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b268c0b...b75cf79. Read the comment docs.

[fredcarle]

Context for other reviewers, the panic was caused by the fact that client.FieldDefinition is used as the key for a map of properties on encodedDocument. For a Go type to be used as a map key it has to be comparable. Within client.FieldDefinition, the DefaultField struct field is of type any and until we added support for JSON default, it only ever contained scalar types. But a default JSON value can be an object which is represented as a map[string]any. A map is not a comparable type and this was causing a runtime panic when trying to create a new doc using the default JSON object. This panic happened in the fetcher code github.com/sourcenetwork/defradb/internal/db/fetcher/fetcher.go:544 when fetching the new document within the creation request.

I'm OK with removing DefaultValue from client.FieldDefinition for now although a better fix might be to change the fetcher code. @AndrewSisley might have a stronger opinion for this one.

[AndrewSisley]

todo: I do not agree with removing it from here - this object is supposed to be a unified object containing the result of local+global configuration and taking it off this breaks that. It will result in weirdness for users (as well as internal code) where some public functions returning Definitions cannot inform users as to what the default value is.

I feel even more strongly about this given that it is described as a somewhat accidental result from the fix, that this just happened to be the quickest way to fix a bug and wasn't really a long term design choice.

Please either discuss the with the whole team over discord or in the standup.

Furthermore, going by:

the panic was caused by the fact that client.FieldDefinition is used as the key for a map of properties on encodedDocument.

The alternative looks pretty simple and non invasive/internal - just use the field ID or name as the map key - they are identifiers and using the whole object sounds unneeded and error prone anyway.

[AndrewSisley]

question: Why are you using both field ID and Name here? At a single point in time either will be unique - you can't two fields with the same fieldID or with the same name.

[nasdf]

Both the field ID and name are used in the downstream dependencies. If we used only one as the map key then we would need to expand the FieldDefinition API to support looking up a field by ID or copy paste a lookup function into a few places. I think having the FieldDefinitionKey is the better alternative.

[AndrewSisley]

There should already be functions to get fields by Name if you use that as the key - I don't think you'd need to add anything to client.

Having a (public) FieldDefinitionKey struct with name and key properties suggests that they need to be combined in order to form a unique key - which is incorrect.

[nasdf]

The DecodeToDoc function in the fetcher doesn't have a reference to the definition so it can't lookup the field ID from the name.

I'm fine with changing the struct name but I don't see any issues with the way I implemented it.

[AndrewSisley]

DecodeToDoc just uses the field ID as an index - we could change that too. Or you could change encodedDocument.properties to a slice containing field name and value.

There are many ways to solve this internal coding problem that don't involve changing the public types, especially not in a way that seems a little misleading.