Add support for generation based on Python Poetry project file #1
Conversation
Current implmentation can only process dependencies that are packaged with a 'metadata.json' file
Deal with CycloneDx obsolete types
Remove .NET 7.0
| internal sealed class PoetryLock | ||
| { | ||
| [DataMember(Name = "package")] | ||
| public List<PoetryLockPackage>? Packages { get; set; } |
There was a problem hiding this comment.
Are all the properties in here nullable because that was the only way to shut the compiler up? Or does this actually reflect a reality about the data: is it really possible that in a poetry lock file, every single one of the properties in the types in this file might not be present.
If some of these properties are in fact mandatory it would be better if they were not nullable here. You might be able to use the required keyword to keep the compiler happy.
I can't tell what serialization mechanism is actually in use here though. It has been a few years since I've seen code using the DataMember attribute. I believe that's typically used with DataContractSerializer, which originated from WCF, and I'm a bit hazy on what the status of that is. (I thought WCF was one of the old school .NET FX features that didn't get fully migrated to modern .NET.)
So this normally makes me think of XML serialization, but I'm wondering if you're using something here that uses these attributes when processing a toml file?
Anyway, the jurassic nature of DataMember means it's all a long way before nullable types were introduced, and I don't think it ever had a thorough nullability API review.
So maybe it's just not possible to get nullability handling to work as well as you'd want here. Really you want a serialization framework to be aware of this stuff, but perhaps that's not an option here.
If required just doesn't work, then instead of marking everything with ?, I'd actually stick a #nullable disable annotations at the top of the file. That tells the compiler that you want the source file to be handled from a "pre-nullability" mindset. It means all the properties would be neither nullable nor non-nullable. They'd be "null-oblivious", and that's typically the least worst option if you find yourself fighting with a serialization mechanism that doesn't understand nullability.
|
|
||
| // Add main component | ||
| var root = context.AddComponent( | ||
| new PoetryComponent(assetFile.Tool.Poetry.Name!, assetFile.Tool.Poetry.Version!, BomComponentKind.Root)); |
There was a problem hiding this comment.
The build errors that GitHub is showing me are occurring here (on my screen, it's showing me a bunch of GitHub Actions failures with Dereference of a possibly null reference) are exactly the kind of problem I'd expect when you try to silence compiler warnings about nullability on data serialization types by slapping a ? on everything. It just pushes the warnings elsewhere in the code.
| @@ -11,7 +11,7 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="CycloneDX.Core" Version="5.4.0" /> | |||
| <PackageReference Include="CycloneDX.Core" Version="7.1.0" /> | |||
There was a problem hiding this comment.
This PR seems to update a bunch of components. Consider adding a description to this PR clarifying whether that's a case of "Updating these while we're in here" or whether these are necessary to achieve the end goal.
|
|
||
| namespace Covenant.Tests; | ||
|
|
||
| public class PythonPoetryTests |
There was a problem hiding this comment.
Is this finished?
This seems to have a very lenient definition of Correctly. Shouldn't these tests inspect the results?
Also, there don't appear to be any test at all for PoetryAnalyzer. Isn't that type the heart of this PR? (I'm currently uncertain on what anlaysis PoetryAnalyzer is actually meant to do, and having some tests would help clarify that.)
| var index = text.IndexOf(':'); | ||
| if (index == -1) | ||
| { | ||
| // Unknown hash format |
There was a problem hiding this comment.
Is this considered normal? This sounds like it might warrant throwing an exception. (And then you'd make the return type string, not string?)
| return Parse(licenseString); | ||
| } | ||
|
|
||
| private static string? ParseLicenseFile(string licensePath) |
There was a problem hiding this comment.
Why is this return type string?? (and not just string) The docs say File.ReadAllText never returns null. (Either it succeeds, or it throws.) Under what circumstances would this return null?
| return File.ReadAllText(licensePath); | ||
| } | ||
|
|
||
| private static string? ParseJsonMetadataFile(string packageMetadataPath) |
There was a problem hiding this comment.
Is this the right name? It appears just to retrieve the license property.
| using (var packageMetadataReader = new StreamReader(packageMetadataPath)) | ||
| { | ||
| var metadata = JsonConvert.DeserializeObject<Dictionary<string, object>>(packageMetadataReader.ReadToEnd()); | ||
| return metadata?["license"]?.ToString(); |
There was a problem hiding this comment.
Again this looks slightly like "keep passing the nullability buck until the compiler stops complaining about this particular code".
The first ? on this line essentially says that you sometimes expect the preceding call to JsonConver.DeserializeObect to return null. That will only happen if the file at packageMetdataPath contains literally the text null.
The only reason DeserializeObect is declared as returning null is because they have to deal with that possibility. The text "null" is, technically, valid JSON. So if the file contains just those four characters, it can't reject it because it's properly formed JSON, and so it returns a .NET null to represent that JSON null.
But it's somewhat rare for that to be non-exceptional. If you would consider it to be a mistake for the file here to contain just the text null, then it would be appropriate to add something like this:
if (metadata is null)
{
throw new ArgumentException("JSON file contains just the text 'null' when an object was expected");
}If you add that before the return statement you'll find you can remove the first of the ? marks because the compiler will know the code can't reach that statement unless metadata is non-null. So it would become this:
return metadata["license"]?.ToString();So now let's talk about that second ?. Is it expected that sometimes there will be no license property in the JSON? If so, then I guess string? is a suitable return type, and you do need that ? in there. But if it's always a mistake for that file not to have a license, and you don't expect to see that in normal usage, consider an exception in that case (at which point you can remove the ? marks on this line and also the return type of the method, and that may in turn remove the need to add a bunch of null handling in code that calls this.)
| } | ||
| } | ||
|
|
||
| private static string? ParseMetadataFile(string packageMetadataPath) |
There was a problem hiding this comment.
Again, this seems just to get a license, so is this the right name for this method?
| @@ -0,0 +1,3 @@ | |||
| using System.Runtime.CompilerServices; | |||
|
|
|||
| [assembly: InternalsVisibleTo("Covenant.Tests")] | |||
There was a problem hiding this comment.
You can generate these with an <ItemGroup> in the csproj containing <InternalsVisibleTo /> elements.
It's a stylistic choice, so if you prefer them in a file, this is fine, but I happen to prefer putting it in the csproj. To me it feels like the csproj is where we declare this component's relationship with other components. E.g., project and package references go in there. This is a relationship pointing in the other direction admittedly, but it still feels like something that belongs in there.
No description provided.