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

fix(jans-orm):sql localizedstring persistence SqlEntryManager #1475

Merged
merged 1 commit into from
May 31, 2022

Conversation

jmunozherbas
Copy link
Contributor

Description

SqlEntryManager: method to get LocalizedString values was modified similar to SqlLdapEntryManager

detected trying to persist sites with mysql DB.

Origin Issue

#260

@jmunozherbas jmunozherbas requested a review from yurem May 31, 2022 04:45
@mo-auto mo-auto added comp-jans-auth-server Component affected by issue or PR comp-jans-orm Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality labels May 31, 2022
@jmunozherbas jmunozherbas marked this pull request as ready for review May 31, 2022 04:46
@jmunozherbas jmunozherbas requested a review from yuriyz as a code owner May 31, 2022 04:46
@sonarcloud
Copy link

sonarcloud bot commented May 31, 2022

[orm] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
81.2% 81.2% Duplication

@jmunozherbas jmunozherbas changed the title fix(jans-auth-server):sql localizedstring persistence SqlEntryManager fix(jans-orm):sql localizedstring persistence SqlEntryManager May 31, 2022
@yuriyz yuriyz merged commit b959b94 into main May 31, 2022
@yuriyz yuriyz deleted the fix-jans-auth-server-sql-localizedstring branch May 31, 2022 06:26
@yurem
Copy link
Contributor

yurem commented May 31, 2022

@yuriyz @jmunozherbas Why we need this method on ORM layer. It not works with DB. The better place for it is in reusable service layer.

@yurem
Copy link
Contributor

yurem commented May 31, 2022

@jmunozherbas in PR description you added reference to #260
But it's about another update. Is there design of this update?

@yurem
Copy link
Contributor

yurem commented May 31, 2022

@qbert2k Can you share design related to LocalizedString?

Why we simply can't use?

    @JsonObject
    @AttributeName(name = "attrbute_name")
    private LocalizedString attributeValueWhichOrmWill convertToJson;

@yuriyz
Copy link
Contributor

yuriyz commented May 31, 2022

Right, this particular PR comes out of what was done in LDAP entry manager for LocalizedString. We are forced to update other entry managers with what we have now. So we have to decide how do we proceed with persistence of LocalizedString. Simple json looks quite feasible. Lets see what @qbert2k say.

@jmunozherbas
Copy link
Contributor Author

@jmunozherbas in PR description you added reference to #260 But it's about another update. Is there design of this update?

I mentioned Issue, because it was the origin of the change, but doesn't have a specific Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-auth-server Component affected by issue or PR comp-jans-orm Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants