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
Merged

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 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.

Loading

@mpro7
Copy link
Contributor

@mpro7 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,

Loading

@subotic
Copy link
Collaborator Author

@subotic 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.

Loading

@subotic
Copy link
Collaborator Author

@subotic subotic commented Oct 4, 2021

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

Loading

@irinaschubert
Copy link
Contributor

@irinaschubert irinaschubert commented Oct 5, 2021

@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.

Loading

@mpro7
Copy link
Contributor

@mpro7 mpro7 commented Oct 6, 2021

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

Loading

@subotic subotic requested a review from mpro7 Oct 6, 2021
mpro7
mpro7 approved these changes Oct 6, 2021

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

@mpro7 mpro7 Oct 6, 2021

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?

Loading

Copy link
Collaborator Author

@subotic subotic Oct 6, 2021

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.

Loading

Copy link
Contributor

@mpro7 mpro7 Oct 6, 2021

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 ???.

Loading

Copy link
Contributor

@mpro7 mpro7 Oct 6, 2021

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.

Loading

Copy link
Collaborator Author

@subotic subotic Oct 6, 2021

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.

Loading

@subotic subotic merged commit d6a0d27 into main Oct 6, 2021
11 checks passed
Loading
@subotic subotic deleted the wip/DEV-78-improve-authentication branch Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants