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

Add handling nested properties during resolving jwt placeholders #1996

Merged

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
3 checks passed
@dimabarbul dimabarbul deleted the jwt-placeholder-handle-nesting branch September 2, 2024 19:11
This pull request was closed.
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