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(authenticator): improve performance #1914

Merged
merged 9 commits into from Oct 6, 2021

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Sep 29, 2021

resolves DEV-78

@subotic subotic self-assigned this Sep 29, 2021
@subotic subotic changed the title refactor(authenticator): cleanup data models fix(authenticator): improve performance Oct 3, 2021
@subotic
Copy link
Collaborator Author

subotic commented Oct 3, 2021

@irinaschubert @mpro7 could you please try this branch and see if the OntologyV2R2RSpec is still failing for you. I've done a bit of optimization. If it is not enough, then I need to do something more radical.

@mpro7
Copy link
Collaborator

mpro7 commented Oct 4, 2021

@subotic I got numerous errors while synchronising which is probably related to failed build, but all tests passed in OntologyV2R2RSpec now,

@subotic
Copy link
Collaborator Author

subotic commented Oct 4, 2021

I did some refactoring and missed a spot that needs changing. I will fix that and then you can try again.

@subotic
Copy link
Collaborator Author

subotic commented Oct 4, 2021

@irinaschubert @mpro7 could you please try again and see if this solves the issue with the one test?

@irinaschubert
Copy link

@irinaschubert @mpro7 could you please try again and see if this solves the issue with the one test?

I tested it locally and the tests run through now.

@mpro7
Copy link
Collaborator

mpro7 commented Oct 6, 2021

@subotic this time both tests and syncing ended w/o errors 👍

@subotic subotic requested a review from mpro7 October 6, 2021 09:43

/**
* Not implemented.
*/
def read(jsonVal: JsValue) = ???
def read(jsonVal: JsValue): PermissionProfileType = ???
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this isn't implemented should be here at all or at least uncommented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question. I can comment it out. It is not used anywhere in the code, or it would throw an exception when used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will do that in my branch, found few more occurrences with ???.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or not, because it's inherited.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are 16 occurrences with ??? inside the whole codebase. If the code is not used anywhere, then the not existing implementation doesn't hurt.

@subotic subotic merged commit d6a0d27 into main Oct 6, 2021
@subotic subotic deleted the wip/DEV-78-improve-authentication branch October 6, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants