[#2141] Use optionals for return values that may be null to enforce a client check

[georgetayqy]

Part of #2141

Proposed commit message

Currently, `null`s are used within some parts of the code. While it is
not a bad practice to use `null`s within the code, it can be
problematic when `null` values are present in places where `null` is
not allowed.

This PR aims to begin the implementation of `Failable` monad,
unit tests for `Failable` monad, as well as the refactoring of
some areas of the code where `null` is explicitly used as predicates or
as a flag for starting other processes.

Other information

Main Rationale Behind FailableOptional Monad

Currently, there are chunks of code whereby nulls are used as a flag or are part of a process whereby exceptions are thrown or messages are logged. The built-in Java Optional class (in Java 8) does not support some of the operations or require lengthy workarounds to replicate the behaviour we want directly using Optional.

For example, consider CommitInfoAnalyzer::analyzeCommit:

public CommitResult analyzeCommit(CommitInfo commitInfo, RepoConfiguration config) {
        // truncated
        ZonedDateTime date = null;
        try {
            date = ZonedDateTime.parse(elements[DATE_INDEX], GIT_STRICT_ISO_DATE_FORMAT);
        } catch (DateTimeParseException pe) {
            logger.log(Level.WARNING, "Unable to parse the date from git log result for commit.", pe);
        }

        // Commit date may be in a timezone different from the one given in the config.
        LocalDateTime adjustedDate = date.withZoneSameInstant(config.getZoneId()).toLocalDateTime();

        // truncated

        return new CommitResult(author, hash, isMergeCommit, adjustedDate, messageTitle, messageBody, tags,
                fileTypeAndContributionMap);
    }

Using null in this manner is dangerous if DateTimeParseException is thrown, since date would have the value null, and the subsequent invocation of date.withZoneSameInstant would fail due to a NullPointerException. Optional could be used here, but it is rather clunky because Optional's interface does not handle the production of values that might throw exceptions. Moreover, the other mapping and filtering methods do not allow lambdas or methods which throw exceptions.

The use of FailableOptional can help solve this, while mitigating the risk of invoking methods on null, by wrapping up the function call that potentially throws an exception, and working with it as if it had not thrown, shielding the user from the complexity of handling the null values.

Consider this rewritten chunk of code:

public CommitResult analyzeCommit(CommitInfo commitInfo, RepoConfiguration config) {
        // truncated
        // safe map since ZonedDateTime::now returns non-null
        Failable<LocalDateTime> date = Failable
                .ofNullable(() -> ZonedDateTime.parse(elements[DATE_INDEX], GIT_STRICT_ISO_DATE_FORMAT))
                .ifFail(x ->
                        logger.log(Level.WARNING, "Unable to parse the date from git log result for commit.", x))
                .resolve(x -> {}, ZonedDateTime.now())
                .map(x -> x.withZoneSameInstant(config.getZoneId()).toLocalDateTime());

        // truncated

        return new CommitResult(author, hash, isMergeCommit, date.get(), messageTitle, messageBody, tags,
                fileTypeAndContributionMap);
    }

This following implementation would not be subjected to the above buggy behaviour, as the potentially throwing method is wrapped into a Fail-ed instance of Failable, and subsequently recovered into a valid default LocalDateTime value.

So far, Failable supports the usual operations that Optional can do (identity (of, ofNullable), mapping (map, flatmap) and filtering (filter)), and support new operations that might be useful in our codebase (handling methods that might throw exceptions when executed but otherwise returns a value, etc.).

So far, the following methods have been implemented:

Identity

• of
• ofNullable
• success
• empty
• fail
• isPresent
• isAbsent
• isFailed

Mapping

• map
• unfailableMap
• flatMap
• unfailableFlatMap

Filtering

• filter

Getters

• get
• orElse

Misc. methods

• ifPresent
• ifAbsent
• ifFail
• resolve
• ifFailThenThrow
• failWith

---

Here are some of the main processes that we need to complete to close out this Issue/PR. The relevant PRs which fulfil the tasks are included in the parenthesis following the task name below:

• Implement Failable monad (#2144)
• Implement test cases for Failable (#2144)
• [IN PROGRESS] Identify areas where Failable can be used (#2144)
• [IN PROGRESS] Implement Failable in key areas identified (#2144)
• [IN PROGRESS] Update overall test cases (#2144)
• Bring code coverage back up to acceptable levels

[georgetayqy]

In line with keeping PRs small and working on issues incrementally, I have decided to stop working on the implementation of FailableOptional for now and limit this PR to a proof of concept / potential implementation of the Optional alternatives we can use.

Future PRs will look into the implementation of FailableOptional in other places in the codebase wherever needed.

[gok99]

Just an initial thought: It would be nice if exceptions were part of the Failable optional type (e.g FailableOptional<LocalDateTime, DateTimeParseException>) which indicates that there exists the possibility of a throw which is still unhandled. Composing FailableOptionals with different unhandled exceptions should produce a FailableOptional with the sum type of these exceptions (this may be hard to do with Java 8/11). Handling the error, then gets rid of the exception from the type.

Inspiration for monads of this kind is from Scala Zio and effect-ts.

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[github-actions]

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[github-actions]

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.

[github-actions]

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

[github-actions]

This PR was closed because it has been marked as stale for 7 days with no activity.
Feel free to reopen this PR if you would like to continue working on it.