WIP - Briefcase code design discussion

[ggalmazor]

Seeing that we're starting to see an increased interest on Briefcase, I think it's a good time to document the high-level design and implementation strategy we've been trying to follow while dealing with new features and bug fixes in Briefcase.

This would serve two purposes:

• Document actual design decisions and their roadmap to make an easier onboarding.
• Serve as common ground for further discussion and improvement.

Background

• This section would explain why we have embarked on some of the refactoring efforts since 1.9.0

Design goals

• This section would explain the reasons why we want to invest any effort in improving the design

General design rules

• This section would list some general design rules like:
  • Testability, unit testing, and end-to-end testing
  • Separation of vertical (business) and horizontal (infrastructure) slices
  • Self-similarity: Matrioska design, UI components
  • Coupling and cohesion - Single Responsibility principle
  • Explicit side-effects and immutability

Roadmap

• This section would have subsections regarding specific Briefcase design topics

Roadmap topic: XYZ

Current flaws and design goals

• This subsection explains current flaws in XYZ and goals of the design

Next

• This subsection would be a sequence of high-level design changes we want to perform

Strategy

• This subsection would explain how and when we're going to work on these changes and if they have to be coordinated with any other change

Error handling, logging, and reporting

Current flaws

• Use of checked exceptions
• All layers handle all exceptions with all possible outcomes
• No clear rules for exception escalation and program termination
• No clear boundary between domain and third-party libraries
• No clear rules for logging and reporting of errors

Design goals

• We use logs for debugging purposes
• We use Sentry for (anonymous, sanitized) error reporting
• Our code only throws unchecked exceptions
• Our code catches third-party exceptions (checked and unchecked) and wraps them into own unchecked exceptions.
• Catching an exception (non-third-party) always implies stopping its escalation (we don't throw it again) and:
  • Return a default value (this is often called to "recover an exception")
  • Produce UI/CLI feedback
  • Log about it using the correct logging level, attaching the exception
• There are no other reasons to ever catch an exception, except in the Launcher
• Uncaught exceptions escalate to the topmost level (the Launcher) where they're caught, logged as errors and then the app is gracefully stopped.

Low-level code

• Logs mainly on debug level
• Log lines include sensible information
• Log lines give useful context aimed to help while solving a problem

App level code

• Logs mainly on info and warn level
• Log lines give context for debug level log lines

Launcher (topmost level)

• Logs mainly on error level
• Logs all exceptions that reach this level
• Sends sanitized error reports to Sentry

Following this schema, only the Launcher interacts with Sentry because any exception that won't reach it is already being managed by some other part and it won't represent a bug.

This also implies that our exceptions have some degree of knowledge about the information they hold, to be able to sanitize them before sending them as a Sentry error report.

Next

• [ ] Send Sentry reports on a catch-all block in Launcher class
• [ ] All verticals (push, pull, export, settings) should follow escalation rules
• [ ] Horizontal packages should follow escalation rules

Strategy

• WIP

Form & submission parsing, and metadata for Briefcase

Current flaws

• FormDefinition, Model, Submission and SubmissionMetaData have arbitrary members supporting only the export operation.
• There is no lightweight unified Form metadata abstraction
• IFormDefinition has three different incompatible subtypes BriefcaseFormDefinition, RemoteFormDefinition and OdkCollectFormDefinition.
  • The former is the one Briefcase relies on for normal operation (exception export op).
  • The latter two are used to pull forms only.
  • All these classes are heavyweight and lack of simple construction methods (all rely on file parsing)
  • All depend on JR abstractions, meaning that a change in JR will require a change on them

Design goals

• The domain should have a lightweight, universal abstraction for form and submission metadata
• These abstractions should provide an API adapted to the problems Briefcase needs to solve.
• Even though values for these abstractions will come from JR, internals should be as decoupled as possible.
  • It's hard to think that we could be decoupled from concepts related to JR's data schema, like DataType, or QuestionDef, for example
  • It would be easy to be decoupled from JR internal XML and DAG representations like TreeReference, for instance

Next

• [ ] Adapt the pull and push ops to stop using BriefcaseFormDefinitions in lieu of a more lightweight, JR agnostic abstraction based or inspired on FormDefinition and Model

Strategy

• WIP

Concurrency and events in Briefcase

Current flaws

• Briefcase uses an old EventBus implementation
  • It doesn't leverage new concurrency models available in Java8
  • It's tightly coupled with Swing's threading model, which is not best suited for CLI ops
  • It has no thread-safe operation and can block the main thread when saturated
  • Event subscription requires annotations, which makes code maintenance and refactoring work harder.
• It doesn't enforce a universal model for concurrency
  • Verticals and horizontal code use a mix of concurrency models: Swing event dispatch thread, swing worker threads, ad-hoc Executor services, ad-hoc Thread pool factories, and parallel streams
• Uses a custom TerminationFuture system to stop "background tasks"
• There are no clear rules for how concurrent code must behave or how errors should be addressed
• There are no clear rules for how different components communicate between themselves. A mix of callbacks, events and tight coupling is used.

Design goal

• Concurrent domain operations should adopt a universal concurrency system
  • It should be based on Java8's parallel streams, ForkJoinPool and CompletableFutures for background task cancellation support.
• Briefcase should have its UI/CLI agnostic, threadsafe domain events system (supported by third-party libraries if needed)
• If CLI and UI layers need to use their respective concurrency/events system, they should be adapted to the domain's concurrency/events system, not the way around.
• Closely related component hierarchies (like mother-children) should reflect their relation by using explicit, tightly coupling callback passing methods.
• Communication between non-related components should be supported by domain event subscription and broadcasting

Next

• [ ] Search candidates to replace the EventBus
• [ ] Migrate all events to a new library
• [ ] Migrate pull and push operations to Streams
• [ ] Transform pull, push and export Streams to CompletableFuture streams that could be aggregated and canceled mid-air

Strategy

• WIP

[ggalmazor]

@opendatakit-bot label "needs discussion"