Make fields on common structs pub

[TimDiekmann]

🌟 What is the purpose of this PR?

For the same reason as blockprotocol/blockprotocol#987 this PR makes structs, which are only a collection of information (like OntologyVertexId, or EntitMetadata) publicly accessible. With the linked PR it's also possible to properly implement From implementations for VersionedUri.

These changes makes a few accesses a little bit more verbose when otherwise chaining is possible, however, in other cases, this greatly improves readability, especially when type hints are available.

I planned to do these changes as a drive-by to my next PR but have split the changes off to make reviewing easier.

🚫 Blocked by

• Make fields in VersionedUri public blockprotocol/blockprotocol#987

🔍 What does this change?

Please see the list of commits to see the changes

This PR also implements FromSql for more types, which previously we didn't because from just looking at the code (without typehints), it was not possible to see, if the parameters returned from row.get(N) were passed to the correct location. With these changes it's now required to type the parameter explicitly, so it's easy to check if a call was made correctly (the parameter passed to get() can't be checked at compile time).

Example:

< let owned_by_id = OwnedById::new(row.get(owned_by_id_index));
< let entity_uuid = EntityUuid::new(row.get(entity_uuid_index));
< let updated_by_id = UpdatedById::new(row.get(updated_by_id_index));
< 
< Ok(Entity::new(
<     properties,
<     link_data,
<     EntityRecordId::new(
<         EntityId::new(owned_by_id, entity_uuid),
<         EntityEditionId::new(row.get(edition_id_index)),
<     ),
<     EntityVersion {
<         decision_time: row.get(decision_time_index),
<         transaction_time: row.get(transaction_time_index),
<     },
<     entity_type_uri,
<     ProvenanceMetadata::new(updated_by_id),
<     // TODO: only the historic table would have an `archived` field.
<     //   Consider what we should do about that.
<     row.get(archived_index),
< ))
---
> Ok(Entity {
>     properties,
>     link_data,
>     metadata: EntityMetadata {
>         record_id: EntityRecordId {
>             entity_id: EntityId {
>                 owned_by_id: row.get(owned_by_id_index),
>                 entity_uuid: row.get(entity_uuid_index),
>             },
>             edition_id: row.get(edition_id_index),
>         },
>         version: EntityVersion {
>             decision_time: row.get(decision_time_index),
>             transaction_time: row.get(transaction_time_index),
>         },
>         entity_type_id,
>         provenance: ProvenanceMetadata {
>             updated_by_id: row.get(updated_by_id_index),
>         },
>         archived: row.get(archived_index),
>     },
> })

[codecov]

Codecov Report

Attention: Patch coverage is 86.99690% with 42 lines in your changes missing coverage. Please review.

Project coverage is 58.41%. Comparing base (1e93d77) to head (4105509).
Report is 3341 commits behind head on main.

Files with missing lines	Patch %	Lines
...raph/src/api/rest/utoipa_typedef/subgraph/edges.rs	0.00%	8 Missing ⚠️
...h/src/api/rest/utoipa_typedef/subgraph/vertices.rs	0.00%	6 Missing ⚠️
...-graph/lib/graph/src/shared/identifier/ontology.rs	37.50%	5 Missing ⚠️
apps/hash-graph/lib/graph/src/store/fetcher.rs	0.00%	4 Missing ⚠️
apps/hash-graph/lib/graph/src/store/postgres.rs	81.81%	4 Missing ⚠️
...graph/src/store/postgres/ontology/property_type.rs	50.00%	4 Missing ⚠️
...h-graph/lib/graph/src/ontology/domain_validator.rs	0.00%	3 Missing ⚠️
apps/hash-graph/lib/graph/src/shared/provenance.rs	70.00%	3 Missing ⚠️
apps/hash-graph/lib/graph/src/knowledge/entity.rs	75.00%	2 Missing ⚠️
...pps/hash-graph/lib/graph/src/api/rest/data_type.rs	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2081      +/-   ##
==========================================
- Coverage   58.51%   58.41%   -0.11%     
==========================================
  Files         285      285              
  Lines       21343    21289      -54     
  Branches      420      420              
==========================================
- Hits        12489    12435      -54     
  Misses       8849     8849              
  Partials        5        5              

Flag	Coverage Δ
backend-integration-tests	3.66% <ø> (ø)
hash-graph	68.32% <86.99%> (-0.14%)	⬇️
unit-tests	1.48% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[indietyp]

I like this! I wonder if rust-lang/rust#105077 might be of use.

[TimDiekmann]

I like this! I wonder if rust-lang/rust#105077 might be of use.

I'm really looking forward to that RFC, thanks for linking this here!

[Alfred-Mountfield]

I'm not really a fan of making the fields on EntityMetadata pub, the metadata shouldn't be mutable (and while I appreciate we can handle that by just not having const mut entity_metadata ..., not having any way of setting inner fields after construction is just more idiomatic and consistent with the rest of our code).

I don't think the gains in two functions of having field: x on the constructor is worth it, that explicitness is provided by IDEs with parameter hints already, and all of the types are distinct so you can't put them in in the wrong order.

We also don't have a need (to my knowledge) of destructuring these

[TimDiekmann]

Thanks, @Alfred-Mountfield, I appreciate the concern. I have reverted changes to OntologyElementMetadata, EntityMetadata, and ProvenanceMetadata and added a TODO to track the RFC, which was linked by @indietyp.

[trunk-io]

😎 Merged successfully (details).