joernio / joern

Open-source code analysis platform for C/C++/Java/Binary/Javascript/Python/Kotlin based on code property graphs. Discord https://discord.gg/vv4MH284Hc
https://joern.io/
Apache License 2.0
2.05k stars 278 forks source link

[Documentation] API Guidelines #3889

Open DavidBakerEffendi opened 10 months ago

DavidBakerEffendi commented 10 months ago

Problem

Joern does not have a fixed protocol for minor/major releases, nor is explicit on what is core/stable APIs. These issues give users who develop with Joern as an external dependency no reliable guideline on maintaining their project with the nightly release schedule.

There is no problem when an API is added, but rather, what APIs should users consider reliable.

Solution

We require both freedom to develop and modify code in Joern, while also having guidelines when touching code considered "stable". This issue is the beginning of an open dialogue on what that should look like, and get some hands on.

Stable APIs

Here is a list of paths containing classes exposing public methods/functions that I think should be considered "stable". @bbrehm suggested we whitelist these, instead of blacklist, i.e., we have a set of stable APIs, and the rest should either be considered internal or experimental.

I will update this list as the discussion proceeds.

Passes

Utilities

Query Steps

This commit has been reported as largely breaking to some users of Joern, and a regression in terms of interoperability in other JVM languages. It would be good to define some stable query items.

Suggested Protocol

Ideally, when a stable API is modified or removed, it should be marked deprecated and provide advice on how to migrate to the new API. This can then be followed up with a minor version bump and scheduled for removal on the next major version bump.

There is already an effort to maintain descriptions of these changes in the CHANGELOG.md but it has no formal protocol.

Documentation

Once this issue is concluded with actionable steps, we should move the results to the documentation to publicly declare the guidelines. A nice-to-have would be a CI/CD step to flag when a stable API is touched, but this may be a bit more complicated.


Inviting the following to the discussion, however anyone is welcome to provide input: @fabsx00 @ml86 @max-leuthaeuser @pandurangpatil @tuxology @bbrehm @mpollmeier

bbrehm commented 10 months ago

Thanks for opening this discussion! Minor points I'd like to add from discord:

  1. We cannot reasonably consistently use JVM public / private / package private in order to describe API stability. This is because we do not have a real mono-repo -- there is a plethora of repositories and sub-projects, by now spread out over multiple companies, who are using joern things; much of that code is proprietary and non-public. Apart from open-source joern, I'd like to stress qwiet (formerly shiftleft) and whirlylabs.
  2. The current informal model is the following: There is an informal understanding which code is "stable public API". Code written against that should not break (but we make no attempt at binary compatibility). Most things are considered "internal"; this means that breaking changes are possible, but someone must go through all the downstream / usage-sites and adapt them to all breaks. Responsible for that is the person who merged the original commit (or one of the reviewers) -- typical "if you break it then you fix it".
  3. There are three immediate problems apparent from this model: First, this informal understanding can sometimes diverge, even between core developers. Second, this informal understanding is not legible to outside users. Third, "if you break it then you fix it" only works for usage sites that are visible to the person breaking it. Same problem as with Linux kernel: If you don't upstream a kernel module, then it will rapidly lose compatibility, leading to things like the android hellscape. The issue is that proprietary confidential code cannot be visible to people who are outside of the relevant NDAs. I believe that @fabsx00 is currently the only person who is structurally / legally capable of fixing both whirlylabs and qwiet downstream. Maybe we should have a big NDA-signing party?
DavidBakerEffendi commented 10 months ago

@bbrehm Sounds like some kind of Joern Enterprise solution is on its way soon šŸ˜‰

Keeping up with changes

To me, it sounds like anything outside joern.io, private or public, could soon become overwhelming for a contributor to consider. A test-API that companies can subscribe to could one solution for signalling a breaking change, but also cripple the speed of changes.

We could also look at a develop and release branch, added complexity but may help automate major/minor releases, as well as give users of Joern the option to choose the cadence/change rate they're willing to keep up with.

Changelog

(An extension of the above) Automating a changelog from the descriptions of commits since the last version may help users depending on Joern debug an API change or new protocol.

Binary Compatibility

This is an interesting topic, as this may also extend past JVM languages. In terms of the schema, we've had things such as cpg.proto.

It would be good to define a level of compatibility features we're willing to maintain, and others we don't.


So much DevOps in the future of Joern...

bbrehm commented 10 months ago

(An extension of the above) Automating a changelog from the descriptions of commits since the last version may help users depending on Joern debug an API change or new protocol.

There is a qualitative difference between breaks that require adaptations on the minor refactoring level (example: function X has been renamed), and breaks that require major rewrites of entire components (example: it used to be possible to do low-latency writes to the graph via legacy tinkerpop odb APIs -- yeah, nope, in the medium future you need to use the cpgpass infra and batch updates or face a 1000x slowdown)

In other words, we need both a changelog for blog-post length descriptions of architectural changes, and a changelog for minor stuff. Such a thing cannot be auto-generated and we must prevent noisy minor stuff almost nobody should care about from leading people to miss the big things.

Due to the noise issue, I am quite against auto-generated change-logs. Commit messages fulfill this function already.

DavidBakerEffendi commented 10 months ago

In other words, we need both a changelog for blog-post length descriptions of architectural change

On that note, the new Hugo-based blog component of the Joern website is nearly ready, and should enable some more accessibility on publishing updates on a more official forum.

Due to the noise issue, I am quite against auto-generated change-logs. Commit messages fulfill this function already.

You make a good point. ScalaDocs could also provide some overview of API changes across versions - and can synchronize with minor releases.

maltek commented 10 months ago

Another aspect: how do we want to agree on changes to stable APIs? I feel rigorous documentation is less important there than discussions before-hand so that no stake-holders have a bad surprise.

E.g. we could agree to announce potentially breaking changes in a dedicated discord channel, and only merge when no objections have been raised within some time frame that should be short enough to allow us to get things done but long enough that everybody has a chance to review how much they'll be affected.

DavidBakerEffendi commented 10 months ago

About 2 weeks have now passed since the issue was raised, and I think these are reasonably the next actionable steps:

The above can be aided with a script that parses git diff to detect if a class on a stable API is being modified. Each action below could ping the discord channel and link the PR, so that stakeholders can comment directly on it.