Generate required properties as non-nullable #160

alnkesq · 2025-01-14T19:52:11Z

When you navigate through the properties of an ATProto response, all of them will be set to nullable, even if the lexicon claims they're required.
This causes either many warnings, or desensitization by using ! everywhere (even in cases where the property is legitimately optional)

Clients and PDSes generally reject records with missing required fields, so there shouldn't be many broken records in the wild (and in those rare cases, NullReferenceExceptions are probably fine, since our input is literally bad data).

I re-ran the code generation tool (at least for the atproto lexicon) and everything compiles as expected (I didn't include it here since it would've been very noisy for an initial draft)

This is what inspired me to write this PR: alnkesq/AppViewLite@8debd3e

alnkesq · 2025-01-14T19:54:22Z

tools/FFSourceGen/PropertyGeneration.cs

@@ -350,7 +350,12 @@ private string GetPropertyType(string className, string name, PropertyDefinition
            throw new InvalidOperationException("Base Type is null or empty.");
        }

-        return $"{baseType}?";
+        if (ownerClass?.Definition?.Required.Contains(jsonName!) == true && baseType != "DateTime")


I left DateTime? as always nullable, since:

Zero makes semantically less sense, compared to for example integers

It would've caused various compilation errors (particularly around createdAt ?? DateTime.UtcNow)

alnkesq · 2025-01-14T19:55:20Z

tools/FFSourceGen/PropertyGeneration.cs

@@ -318,7 +318,7 @@ private string GetUnknownObjectType(PropertyDefinition property)
        return "ATObject";
    }

-    private string GetPropertyType(string className, string name, PropertyDefinition property)
+    private string GetPropertyType(string className, string name, PropertyDefinition property, ClassGeneration? ownerClass = null, string? jsonName = null)


There are 2 other call sites against this, I left the old behavior for now.

The two other call sites are respectively for List<>, where ? is already always removed, and implementation statements that don't depend on nullness, so there's nothing to actually change.

alnkesq · 2025-01-14T19:59:01Z

tools/FFSourceGen/Program.cs

@@ -2252,7 +2252,8 @@ private void GenerateHeader(StringBuilder sb)
        sb.AppendLine();

        // Add #nullable
-        sb.AppendLine("#nullable enable");
+        sb.AppendLine("#nullable enable annotations");
+        sb.AppendLine("#nullable disable warnings");


I was considering whether to use required Example example { get; set; }, but that would break existing users that set the fields one by one instead of using an initialization block.

The warnings for generated files are disabled, since the (CBORObject) constructor only sets the fields that are actually in the CBOR.

drasticactions · 2025-01-15T08:18:31Z

src/FishyFlip/OpenGraphParser.cs

+        // These three fields are all required, according to the lexicon.
+        if (urlEmbed == null || title == null || description == null)
+        {
+            return null;


If it fails to find any one of these properties, it should throw an Exception. Otherwise you'll end up with a null object with no idea why it failed.

GenerateEmbedExternal already returns null to represent missing or incorrect OpenGraph tags, and bskycli prints an error message in such case.

drasticactions · 2025-01-15T09:40:34Z

Can you rebase on Main and run build_bindings.sh please? It should be buildable now (I also really really have to make FFSourceGen less... like it is. I apologize, It's a mess.) and once you're ready, you can bump the bindings along with these changes.

Considering JsonRequired is there (It should fail on serialization/deserialization if those values are not there), it follows that those values should also not be nullable. From memory, I think I kept it this way because some users had complained in the past about the required constructors, but yeah, it makes far more sense to make it forced.

drasticactions · 2025-01-15T15:05:21Z

@alnkesq What OS did you run the build_bindings.sh script on? Windows, Mac, Linux?

I run it on my Mac. I think there's an issue with how the files get generated through the order in operation of what gets generated first.

ziodotsh/lexicons#1

For bskycli, the atfile binding is broken. That's because the id in the actual lexicon is duped (which is a bug in their code), and lock.json got generated first when you ran it, but when I ran it upload.json got it first (my bug). I also saw your PR, you got to it before I could, lol.

Also AppPassword and AppPasswordDef got switched around. Now, that's a hack on my part, since they duplicated names and they return different values in the lexicon, so I had to get crafty. I can fix that.

drasticactions · 2025-01-15T15:18:08Z

@alnkesq Okay, I think I figured it out:

FishyFlip/tools/FFSourceGen/Program.cs

Line 113 in 45e1c99

    
           var files = Directory.EnumerateFiles(lexiconPath, "*.json", SearchOption.AllDirectories).ToList();

If you update this to OrderByDescending

    private async Task GenerateClasses(string lexiconPath)
    {
        var files = Directory.EnumerateFiles(lexiconPath, "*.json", SearchOption.AllDirectories).OrderByDescending(n => n).ToList();
...

Then I think it should normalize the list and be consistent. In your branch if I set it to OrderBy then the generated result matches one to one with your PR. When I use OrderByDescending it "fixes" the binding by going back to the original behavior, and that will update ATFile and AppPassword/AppPasswordDef back to their original code.

drasticactions · 2025-01-15T15:25:04Z

For the tests failing, A bunch of JsonConverter values are now not being generated, causing it to fail.

https://github.com/drasticactions/FishyFlip/blob/45e1c99586e64ff7a6f926353badfa194f408dba/src/FishyFlip/Lexicon/Com/Atproto/Repo/DescribeRepoOutput.g.cs#L55C9-L55C76

alnkesq · 2025-01-15T15:27:26Z

Yes I had noticed that problem (still solving other issues)

        if (defJsonPath.Replace("\\", "/").EndsWith("/blue/zio/atfile/lock.json"))
        {
            // There's a typo in the original file.
            // The generation output depends on the file system enumeration order
            // since there's a second blue.zio.atfile.upload
            // https://github.com/ziodotsh/lexicons/pull/2
            schemaDocument.Id = "blue.zio.atfile.lock";
        }

alnkesq · 2025-01-15T15:28:39Z

I'm running it on WSL, with NTFS enumeration order (lexicographical)

alnkesq · 2025-01-15T15:30:37Z

I'm not a bash expert, but build_bindings.sh only works on WSL/Linux/Bash if I make these changes:

        if [ "$target_dir" == "whtwnd-whitewind-blog" ]; then
-            REPO_DIRS+=("$(PWD)/../fflexicons/$target_dir/lexicons/com/whtwnd")
+            REPO_DIRS+=("$PWD/../fflexicons/$target_dir/lexicons/com/whtwnd")

and so on.

Otherwise it looks for a binary called PWD
Should I commit them?

drasticactions · 2025-01-15T15:32:49Z

https://github.com/drasticactions/FishyFlip/blob/develop/tools/FFSourceGen/Program.cs#L2070-L2123

This is why the JSONConverters are missing, it was me being really lazy and just checking raw strings, including the nullable ?. Go figure!

If you mod that to be inclusive for both nullable and non-nullable types it should work, I would just hack it to be a contains

if (property.RawType.Contains("FishyFlip.Models.ATUri"))

then it should be fine. This is what I get for "stream of consciousness" programming.

e. Actually:

FishyFlip/tools/FFSourceGen/Program.cs

Lines 2070 to 2097 in 45e1c99

    
           if (property.PropertyDefinition.Type == "array" && !property.IsBaseType) 
        
           { 
        
               if (property.RawType == "FishyFlip.Models.ATUri?") 
        
               { 
        
                   sb.AppendLine( 
        
                       $"        [JsonConverter(typeof(FishyFlip.Tools.Json.GenericListConverter<{property.RawType}, FishyFlip.Tools.Json.ATUriJsonConverter>))]"); 
        
               } 
        
               else if (property.RawType == "Ipfs.Cid?") 
        
               { 
        
                   sb.AppendLine( 
        
                       $"        [JsonConverter(typeof(FishyFlip.Tools.Json.GenericListConverter<{property.RawType}, FishyFlip.Tools.Json.ATCidJsonConverter>))]"); 
        
               } 
        
               else if (property.RawType == "FishyFlip.Models.ATHandle?") 
        
               { 
        
                   sb.AppendLine( 
        
                       $"        [JsonConverter(typeof(FishyFlip.Tools.Json.GenericListConverter<{property.RawType}, FishyFlip.Tools.Json.ATHandleJsonConverter>))]"); 
        
               } 
        
               else if (property.RawType == "FishyFlip.Models.ATDid?") 
        
               { 
        
                   sb.AppendLine( 
        
                       $"        [JsonConverter(typeof(FishyFlip.Tools.Json.GenericListConverter<{property.RawType}, FishyFlip.Tools.Json.ATDidJsonConverter>))]"); 
        
               } 
        
               else if (property.RawType == "FishyFlip.Models.ATIdentifier?") 
        
               { 
        
                   sb.AppendLine( 
        
                       $"        [JsonConverter(typeof(FishyFlip.Tools.Json.GenericListConverter<{property.RawType}, FishyFlip.Tools.Json.ATIdentifierJsonConverter>))]"); 
        
               } 
        
           }

This array code can be gotten rid of. It's never invoked and would fail to be compiled if run.

drasticactions · 2025-01-15T15:36:38Z

REPO_DIRS+=("$PWD/../fflexicons/$target_dir/lexicons/com/whtwnd")

Changed it on my Mac and it worked fine. You can commit it.

…macOS Bash

This reverts commit beeba28.

alnkesq · 2025-01-15T15:59:02Z

The lexicon fix ziodotsh/lexicons#2 has already been merged, so I removed the workaround.

drasticactions · 2025-01-15T16:02:58Z

Ran the Authed Tests and they came back green. Once this builds I'll merge it.

Since it's a breaking change I'm not going to push this to stable right away. I'll test it against some other libraries and apps to see what the damage will be. Since those values were required anyway for the APIs to work It shouldn't be bad, but just in case.

alnkesq · 2025-01-15T16:09:49Z

Warnings aside, the only breaks I had in my own code were old Nullable<TStruct>.Value accesses, for example Facets.Index.ByteStart.Value

If it's too bad we can change this PR to only work on reference types.

drasticactions · 2025-01-15T16:24:36Z

Turns out that at some point in tagging 3.2.2 I broke GitVersion so it was generating that last build as 3.3.0-alpha2 so it wasn't uploading

https://www.nuget.org/packages/FishyFlip/3.4.0-alpha.0

I ended up tagging 3.3 to force it for a newer version. I probably screwed something up there, I'll figure that out.

alnkesq commented Jan 14, 2025

View reviewed changes

drasticactions reviewed Jan 15, 2025

View reviewed changes

Generate required properties as non-nullable

1ecb92d

alnkesq force-pushed the nonnullable-properties branch from 4dc73a0 to 248b458 Compare January 15, 2025 14:21

alnkesq added 5 commits January 15, 2025 16:38

Add missing PropertyNameToCSharpSafeValue calls

6eceefe

Workaround for lexicon bug

beeba28

Fix PWD being interpreted as a command rather than a variable on non-…

53ea04c

…macOS Bash

Fix non-nullable types were not matched for JsonConverter

213fd43

Update bindings

73295f4

alnkesq force-pushed the nonnullable-properties branch from 248b458 to 73295f4 Compare January 15, 2025 15:47

alnkesq marked this pull request as ready for review January 15, 2025 15:55

Revert "Workaround for lexicon bug"

7ae39e4

This reverts commit beeba28.

drasticactions merged commit 556b622 into drasticactions:develop Jan 15, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate required properties as non-nullable #160

Generate required properties as non-nullable #160

alnkesq commented Jan 14, 2025

alnkesq Jan 14, 2025

alnkesq Jan 14, 2025

alnkesq Jan 15, 2025

alnkesq Jan 14, 2025

drasticactions Jan 15, 2025

alnkesq Jan 15, 2025

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025

alnkesq commented Jan 15, 2025

alnkesq commented Jan 15, 2025

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025 •

edited

Loading

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025

Generate required properties as non-nullable #160

Generate required properties as non-nullable #160

Conversation

alnkesq commented Jan 14, 2025

alnkesq Jan 14, 2025

Choose a reason for hiding this comment

alnkesq Jan 14, 2025

Choose a reason for hiding this comment

alnkesq Jan 15, 2025

Choose a reason for hiding this comment

alnkesq Jan 14, 2025

Choose a reason for hiding this comment

drasticactions Jan 15, 2025

Choose a reason for hiding this comment

alnkesq Jan 15, 2025

Choose a reason for hiding this comment

drasticactions commented Jan 15, 2025 • edited Loading

drasticactions commented Jan 15, 2025 • edited Loading

drasticactions commented Jan 15, 2025 • edited Loading

drasticactions commented Jan 15, 2025

alnkesq commented Jan 15, 2025

alnkesq commented Jan 15, 2025

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025 • edited Loading

drasticactions commented Jan 15, 2025

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025 • edited Loading

alnkesq commented Jan 15, 2025

drasticactions commented Jan 15, 2025

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading

drasticactions commented Jan 15, 2025 •

edited

Loading