Skip to content

Add handling nested properties during resolving jwt placeholders#1996

Merged
thjaeckle merged 2 commits intoeclipse-ditto:masterfrom
dimabarbul:jwt-placeholder-handle-nesting
Sep 2, 2024
Merged

Add handling nested properties during resolving jwt placeholders#1996
thjaeckle merged 2 commits intoeclipse-ditto:masterfrom
dimabarbul:jwt-placeholder-handle-nesting

Conversation

@dimabarbul
Copy link
Contributor

Resolves #1985

@thjaeckle
Copy link
Member

@dimabarbul thanks a lot for the PR.
Will review next week once back from my vacation.

@thjaeckle thjaeckle added this to the 3.6.0 milestone Aug 26, 2024
@dimabarbul
Copy link
Contributor Author

@thjaeckle Give me some time (couple of days or so), please, I want to make another version with HashSet instead of Stream for comparison. I haven't checked performance, but I'm pretty sure, HashSet version should be better.

@dimabarbul
Copy link
Contributor Author

HashSet version is:

    private static Set<String> resolveValues(final JsonValue jsonValue, final JsonPointer jsonPointer) {
        final Set<String> result = new HashSet<>();

        resolveValuesInner(jsonValue, jsonPointer, result);

        return result;
    }

    private static void resolveValuesInner(
            final JsonValue jsonValue, final JsonPointer jsonPointer, final Set<String> result) {
        jsonPointer.getRoot()
                .ifPresentOrElse(
                        root -> {
                            if (jsonValue.isArray()) {
                                jsonValue.asArray().stream()
                                        .forEach(item -> resolveValuesInner(item, jsonPointer, result));
                            } else if (jsonValue.isObject()) {
                                jsonValue.asObject().getValue(root)
                                        .ifPresent(v -> resolveValuesInner(v, jsonPointer.nextLevel(), result));
                            }
                        },
                        () -> {
                            if (jsonValue.isArray()) {
                                jsonValue.asArray().stream().map(JsonValue::formatAsString).forEach(result::add);
                            } else {
                                result.add(jsonValue.formatAsString());
                            }
                        });
    }

I've roughly measured performance of HashSet and Stream versions (using System.currentTimeMillis) and found that time is relatively the same (with stream version slightly better). So I'll leave the PR as is.

@thjaeckle thjaeckle force-pushed the jwt-placeholder-handle-nesting branch from cebc31d to de633fc Compare September 2, 2024 09:16
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimabarbul LGTM 👍

I just rebased from master branch and removed the "mutablity" unit test - as this is no longer possible to do with the updated Java version 21 (which was merged in the meantime on master).

Thanks a lot.

@thjaeckle thjaeckle merged commit bfafdfe into eclipse-ditto:master Sep 2, 2024
@dimabarbul dimabarbul deleted the jwt-placeholder-handle-nesting branch September 2, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

JWT placeholder resolving does not correctly expand nested arrays

2 participants