Fix SCT-3001, SCT-3002 and friends #52
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ommented out code
…ame result fields to align with rest of kbase and be easier to understand, add suffix delimiter and fix index parsing. Most of the functional changes to fix SCT-3001 and SCT-3002.
…fix for the changes.
… to as the "context" (but just passes auth token around); missing access group throw error does not silently succeed; missing profile throws error does not silently succeed; fix dates to be uniform epoch ms; improve (make more uniform and understandable) result keys; refactor key mapping, copying, transformation, exclusion - the old logic was difficult to trace;
…indexed (at least no uniformly) and not supported in the user search tools; if features or subobjects should return to general search, it should be a generic feature without hardcoding of types.
…verted to ms elsewhere
Codecov Report
@@ Coverage Diff @@
## develop #52 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 658 661 +3
=========================================
+ Hits 658 661 +3
Continue to review full report at Codecov.
|
scanon
reviewed
Apr 20, 2021
scanon
reviewed
Apr 20, 2021
tests/integration/data/legacy/search_objects/case-06-response.json
Outdated
Show resolved
Hide resolved
scanon
approved these changes
Apr 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a big PR, as it incorporates the fixes for the original issues reported in SCT-3001 and SCT-3002, a handful of bugs encountered while generating tests for those fixes, and many test changes.
I also conducted some renaming refactoring, because it could be quite difficult to follow the code with some of the key names chosen (e.g. "timestamp", "type").
I wouldn't blame you, dear reviewer, if you asked me to break this up into multiple PRs.
Still, here we go. I'll summarize the changes:
SCT-3001: was caused by in invalid "kbase_id" field, which is essentially an "ups" or object reference. It was resolved by removing the "kbase_id" field, renaming the workspace id, object id, and object version individual fields to be more user friendly, and refactoring the front ends to use the individual fields to create an object reference, rather than the now-absent kbase_id. At the same time, removed the "guid" field, and replaced usages of guid with id (there is no longer a guid, but it is functionally the same as the "id" field)
SCT-3002: was caused by an invalid index name being included in results; rather than the index alias, the concrete alias was being returned. The cause of this was an incorrect parsing of the concrete index name, which is composed of a prefix (search2) and suffix (the index version). The bug was that the parser assumed the prefix and suffix separators are the same, but they are not -- the prefix uses a "." and the suffix a "_". Fixing this required adding the suffix separator constant, adjusting the parsing code, and adjusting tests.
While fixing the above code, it was clear that the key naming, while mostly adhering to the original search1 api where applicable, was confusing and difficult to trace. Generic names like "timestamp", "type" or "version" are meaningless out of context, and others were inconsistent.
In addition, the testing support needed improvement (it still does.) One of the main problem areas was test data and strictness of schemas. Several fields were in the incorrect location, appearing under the type-specific data key rather than in the firmly-typed search results. That is, the type-specific data (the specific fields indexed for an object) is an unchecked object, so fields appearing in it are simply allowed without validation. It so happened that, given the way the search results are transformed from the es results to the api results, it was quite easy to miss fields which should appear in the top level search result rather than in the type-specific data. Those fields, while mandatory, were also not specified as required in the schema.
It took some juggling of schemas, the transformation parser, test data, and the front end code to get that sorted out. But I did it in this PR since to have tests pass I would otherwise have to accept that they were also broken.
So that leads to the final area of change - tests. I found that test data, certainly in the areas I was testing, was minimal, hand-crafted, and not reflective of the real usage of the api. So I rebuilt the test around real, generated data, and re-oriented the mocking from the higher level api to the lower level service api (workspace, user profile service). This allowed me to create a small set of supporting mocks which support the test data (covers all workspaces and user profiles), which can be reused in many tests. Further work is needed in that area to DRY out those tests.