[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [sarif] RE: I don't think we need an "attachment" object any more
I filed Issue #155, “Remove annotation object; use regions instead”. If we agree, I would do it in the same change draft as Issue #137, “Support annotating image attachments”. One more thing: even though attachment is staying around, it’s still reasonable to ask whether its message property really belongs on the file object, next door to file.roles. That’s the question that got me thinking about all this in the first place (see the very first message in this thread). I think that after all attachment.message should stay where it is. It’s possible to imagine attaching the same file to multiple results, each of which has something different to say about it. Larry From: sarif@lists.oasis-open.org <sarif@lists.oasis-open.org> On Behalf Of Larry Golding (Comcast) Michael and I just discussed this at length by phone. TL;DR: I am wrong and Michael is right. The attachment object must not disappear – because in the design for supporting screenshots (#137), we just added annotated regions and rectangles to the attachment object! run =attachments:attachment[] result =attachments:attachment[] attachment =description:message =fileLocation +regions:region[] +rectangles:rectangle[] region +message +rectangle top:integer, left:integer, bottom:integer, right:integer message I had completely forgotten that when I proposed to remove attachment. Sorry! In fact, the thing that should disappear is the annotation object, an abomination that looks like this: location physicalLocation ... annotations:annotation[] ... annotation message locations:location[] See how a location has an array of annotations, and each annotation has an array of locations, each of which has an array of annotations…! Michael and I propose to improve this design by removing the annotation object, and changing the type of location.annotations from annotation[] to region[] (remember that thanks to #137, region has a message property): location physicalLocation ... annotations:region[] ...
The viewer experience might be that when the user double clicks on a result, the viewer opens the result file and puts the cursor at location.physicalLocation. But the viewer also highlights the regions specified by location.annotations[].region. And if the user hovers over one of those regions, the viewer displays location.annotations[].region.message as hover text. Sorry for the confusion. Larry From: sarif@lists.oasis-open.org <sarif@lists.oasis-open.org> On Behalf Of Larry Golding (Comcast) I think you might still be a bit confused 😊 – this proposal has nothing to do with annotations. It does not affect the definition or usage of the annotation object in any way. Are you asking what might happen if the same file appeared as an attachment on two different result objects, and the analysis tool wanted to associate a different message with each of those two usages? From: Michael Fanning <Michael.Fanning@microsoft.com> I have clearly been a bit confused, my apologies. Here is my current open issue: a tool might want to provide two sets of annotations that are associated with a single file in the file table. Can you describe how this would work in the new proposal? If there’s a one-to-one association with a set of annotations and a file, that would be unappealing. From: Larry Golding (Comcast) <larrygolding@comcast.net> To emphasize: the only thing this proposal does is get rid of an object that does nothing useful except carrying a message, and putting that message it what is IMO a more appropriate place (on the file object). From: sarif@lists.oasis-open.org <sarif@lists.oasis-open.org> On Behalf Of Larry Golding (Comcast) Merging threads. Michael wrote: So attachments used to have regions which could be used to annotate a region in the attachment. If simplify to a file location, don’t we lose it? The same file in the table could be annotated in separate ways, so we can use a unique file location to for this. The answer is: No. Attachments have never had regions. An attachment is (and always has been) nothing but a fileLocation (uri + uriBaseId) + a description. The thing that has a region is a physicalLocation. So for example, result.conversionProvenance is an array of physicalLocation objects, so you can see the region(s) of the analysis tool log file(s) that were converted into the SARIF result object. You might be confusing attachment with annotation. From: sarif@lists.oasis-open.org <sarif@lists.oasis-open.org> On Behalf Of Larry Golding (Comcast) This change does not affect annotations. Annotations don’t live on the attachment object, they live on the location object: location physicalLocation ... annotations:annotation[] ... annotation message locations:location[] … which is indeed a little twisted, but anyway, it’s not related to attachments. To your request for a more comprehensive view of the result of my proposal: invocation commandLine:string arguments:string[] ... attachments:fileLocation[] # Instead of attachment[]. result ruleId:string message ... attachments:fileLocation[] # The only other place attachment object was used.
file fileLocation mimeType:string contents:fileContent ... roles:string[] # Since we just put this here... description:message # ... it seems natural for this to be here as well. Larry From: Michael Fanning <Michael.Fanning@microsoft.com> What has happened to the annotations that are associated with an attachment? Can you provide a more comprehensive view of the final type definitions you have in mind? From: sarif@lists.oasis-open.org <sarif@lists.oasis-open.org> On Behalf Of Larry Golding (Comcast) I’m sorry, I was so excited about this idea that I was inaccurate. conversionProvenance is of type physicalLocation[] (and physicalLocation = fileLocation + region), not of type attachment[]. And it’s the invocation object, not the run object, that has an attachments property. But I still think replacing the attachment object with the file.description property is a good idea! From: Larry Golding (Comcast) <larrygolding@comcast.net> Also, I would add a new permittted value analysisToolLogFile in file.roles. From: Larry Golding (Comcast) <larrygolding@comcast.net> As you know, both run and result have a property attachments of type attachment[]. result also has conversionProvenance of type attachment[]. An attachment is nothing but a fileLocation plus a description. The spec says this about description: An attachment object SHOULD contain a property named description whose value is a message object (§3.9) describing the role played by the attachment. The “role”, huh? Be we just added a roles property to the file object! So perhaps the file object is also the natural place for description. Then we don’t need the attachment object, and instead we have: file =role +description:message run: ~attachments:fileLocation[] result: ~attachments:fileLocation[] ~conversionProvenance:fileLocation[]
I stumbled on this as I was starting to write the words for Issue #134, “conversion.analysisToolLogFileLocation should be an array”, which made me look at result.conversionProvenance and wonder why it was an attachment[]. If we agree, I’d like to do this in the same change draft as the one I’m writing for #134, since they both touch result.conversionProvenance. Thoughts? Larry |
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]