Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #63. Fix cross reference logic #64

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

rmouritzen-splunk
Copy link
Contributor

@rmouritzen-splunk rmouritzen-splunk commented Jan 24, 2024

Issue #63. Fix cross reference logic. This logic is used the following pages: dictionary, objects, object detail, profiles, and profile detail. Before this, classes were ignored when Base Event was one of the references. This was presumably done because when Base Event is referenced, it implies all other classes are also referenced.

The prior behavior was that when "Base Event" was a reference, no other classes were shown. With this change, the entire list of classes is shown when Base Event is a reference because, of course, anything that is in Base Event is also in every other class.

The question is: is this helpful? Is showing all classes helpful when Base Event is the one of the references?

Perhaps what we want instead is to show which classes or objects add or override an attribute. Let me know.

(The remainder of this description contains relatively minor details.)

The references are the form, with the 3 sections existing when applicable:

Base Event Class
-----------------
Foo Class
Bar Class
-----------------
Baz Object
Quux Object

Now that Base Event will also appear in the classes section, the naming of event classes now consistently takes the form "Foo Class" instead of "Foo Event". This prevents an awkward "Base Event Event" name. In addition the form "Foo Class" is consistent with the class page, which uses the form "Foo (42) Class" where "Foo" is the class name and "(42)" is the class ID in parenthesis.

Listing "Base Event Class" at the top is redundant, however it does highlight when then Base Event is a reference.

…ges. Before this, classes were ignored when Base Event was one of the references.
@rmouritzen-splunk rmouritzen-splunk changed the title Issue #63. Fix cross reference logic on dictionary and objects pages. Issue #63. Fix cross reference logic Jan 24, 2024
@mikeradka
Copy link
Contributor

mikeradka commented Jan 24, 2024

We have the 1.1 release coming up tomorrow. For the OCSF servers, we should update the docker image while we also update the schema. Are we confident that this could go into the latest docker image for the 1.1 release?

@rmouritzen-splunk
Copy link
Contributor Author

We have the 1.1 release coming up tomorrow. For the OCSF servers, we should update the docker image while we also update the schema. Are we confident that this could go into the latest docker image for the 1.1 release?

There's no need for this with to occur with the 1.1 release. This change is independent.

More broadly, I don't see a need to update the OCSF Server for the 1.1 release. It's already working properly (barring known bugs) with the 1.1.0-dev schema, as far as I know.

@rmouritzen-splunk
Copy link
Contributor Author

Also, @mikeradka, I'm also not sure this change is desirable... See the comment above about how when "Base Event" is referenced, all event classes end are also always listed with this change. I'm not sure that makes sense.

  • Perhaps we should simply change "Base Event" to something like "Base Event (referenced in all event classes)" rather than listing all classes.
  • Also, for attributes (not objects) we might also want to show which class initially defines it, and which classes override it (when applicable).

@rmouritzen-splunk
Copy link
Contributor Author

Per discussion with @pagbabian-splunk, we also want to show where attributes are first included in a class or object, and where they are overridden.

@rmouritzen-splunk
Copy link
Contributor Author

@floydtree and @pagbabian-splunk : this is ready to review

@rmouritzen-splunk
Copy link
Contributor Author

rmouritzen-splunk commented Feb 22, 2024

Summary of pages changed:

  • Classes
  • Class / Base Event
  • Dictionary
  • Objects
  • Object
  • Profiles
  • Profile

Changes / things to review:

  • In all "Referenced By" areas, class names now use the form " Class". Before this change, classes used the form " Event" except for the "Base Event", since that would end up being "Base Event Event". Objects continue to use the form " Object. The items in these lists now use a break (<br>) instead of commas between items.
  • Class page: the tooltips on attributes now show the source using caption (instead of the internal name) as well as showing the class hierarchy when the attribute comes from a parent class.
  • Dictionary page, Objects page, and Profiles page each changed to collapsing "Referenced By" lists.
  • Dictionary page: when "Base Event Class" is referenced, the second section becomes "Referenced by all classes" followed by the list of "updated" classes -- where the attribute was updated after its addition to Base Event.
  • Dictionary page: classes now have a tooltip showing the where the attribute was referenced / updated.
  • Objects page: Classes and objects now have a tooltip showing the attributes of the object's type.
  • Object page: Classes and objects in Referenced By section show the attributes of the object's type.

@floydtree
Copy link
Contributor

This is great, thanks @rmouritzen-splunk

I have added inline comments for a couple of things, here are the general ones -

In all "Referenced By" areas, class names now use the form " Class". Before this change, classes used the form " Event" except for the "Base Event", since that would end up being "Base Event Event". Objects continue to use the form " Object. The items in these lists now use a break (
) instead of commas between items.

Class page: the tooltips on attributes now show the source using caption (instead of the internal name) as well as showing the class hierarchy when the attribute comes from a parent class.

Dictionary page, Objects page, and Profiles page each changed to collapsing "Referenced By" lists.

Dictionary page: classes now have a tooltip showing the where the attribute was referenced / updated.

Objects page: Classes and objects now have a tooltip showing the attributes of the object's type.

These are all awesome, so much cleaner and informative output.

Dictionary page: when "Base Event Class" is referenced, the second section becomes "Referenced by all classes" followed by the list of "updated" classes -- where the attribute was updated after its addition to Base Event.

Can you elaborate on what "Updated In" means? If we take the following as an example, I want to understand the distinction between referenced by vs updated in.

Screenshot 2024-02-22 at 11 13 58

Object page: Classes and objects in Referenced By section show the attributes of the object's type.

Can we make the Referenced By item collapsible? We can default to a collapsed view, and folks can expand if desired.

Screenshot 2024-02-22 at 11 16 07

lib/schema_web/views/page_view.ex Outdated Show resolved Hide resolved
lib/schema_web/views/page_view.ex Show resolved Hide resolved
@rmouritzen-splunk
Copy link
Contributor Author

rmouritzen-splunk commented Feb 22, 2024

@floydtree :

Can you elaborate on what "Updated In" means? If we take the following as an example, I want to understand the distinction between referenced by vs updated in.

"Update in" here means that an attribute's details have changed. This is typically the description field, or additional enum values.This comes from the attribute's _source field. This can actually happen in other situations than when the attribute first appears in "Base Event", however in this issue / PR, Paul was only asking for this information when the attribute was first added in Base Event. (This is the most common case.) You can, though, see the more complete view using the tooltip information on the attribute name on the class page.

I'll try adding an explanation as a tooltip to the "Update in..." collapse link/button thing. This didn't work. Bootstrap doesn't support an element being both a collapse button and a tooltip (maybe because both use the data-toggle attribute).

@rmouritzen-splunk
Copy link
Contributor Author

rmouritzen-splunk commented Feb 22, 2024

@floydtree :

Can we make the Referenced By item collapsible? We can default to a collapsed view, and folks can expand if desired.

I was figuring that these lists are at the end of the page, so having a long list there wouldn't be a problem. It's easy enough to make it collapsible though.

I made the change. It's only on the object and profile pages (the pages for a specific object and specific profile). Check it out. I'm up in the air about it.

@floydtree
Copy link
Contributor

I was figuring that these lists are at the end of the page, so having a long list there wouldn't be a problem. It's easy enough to make it collapsible though.
I made the change. It's only on the object and profile pages (the pages for a specific object and specific profile). Check it out. I'm up in the air about it.

I think the change looks good, the constraints are pulled up and visible by default as well.

@floydtree floydtree merged commit 1119988 into ocsf:main Feb 23, 2024
1 check passed
@rmouritzen-splunk rmouritzen-splunk deleted the issue_63_cross_refs branch February 23, 2024 21:25
@rmouritzen-splunk rmouritzen-splunk restored the issue_63_cross_refs branch February 23, 2024 21:27
@rmouritzen-splunk rmouritzen-splunk deleted the issue_63_cross_refs branch February 23, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants