-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature/spring boot 2.x accumulo 2.1 #18
Conversation
…cleanup to follow)
…tinue test updates and progress.
…ityAgency/datawave-dictionary-service into feature/queryMicroservices
… and removed javax.annotations.Nonnull from test.
…ate and future leaning jakarta (for java 11 and spring 6 compatibility).
…ityAgency/datawave-dictionary-service into feature/queryMicroservices
… up-to-date and future leaning jakarta (for java 11 and spring 6 compatibility)." This reverts commit a2d7e3b.
Updated versions of curator, zookeeper, and ensure in-memory accumulo is included
private static final HashSet<String> RESERVED_COLF_VALUES = Sets.newHashSet("e", "i", "ri", "f", "tf", "m", "desc", "edge", "t", "n", "h"); | ||
|
||
public ModelController(ModelProperties modelProperties, AccumuloConnectionService accumloConnectionService) { | ||
this.dataTablesUri = modelProperties.getDefaultTableName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this should be this.dataTablesUri = modelProperties.getDataTablesUri();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
* @HTTP 500 internal server error | ||
*/ | ||
@Operation(summary = "Insert a new field mapping into an existing model.") | ||
@PostMapping(value = {"/insert", "/import"}) // If we get to change to standard REST, this would just be / and rely on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfinished comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure what was meant by that comment. deleted.
|
||
Set<DESC> getDescriptions(Connection connectionConfig, String fieldName, String datatype) throws Exception; | ||
|
||
void deleteDescription(Connection connectionConfig, String fieldName, String datatype, DESC description) throws Exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems odd that description is supplied here. it looks like it's only used to determine the col vis, which I guess is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to keep making these same kinds of comments, but this isn't new, just a relocation of the following class during Ceria's refactor: https://github.com/NationalSecurityAgency/datawave-dictionary-service/blob/main/service/src/main/java/datawave/microservice/dictionary/data/DatawaveDataDictionary.java
*/ | ||
public Set<Authorizations> getDowngradedAuthorizations(String requestedAuthorizations, DatawaveUserDetails currentUser) { | ||
DatawaveUser primaryUser = currentUser.getPrimaryUser(); | ||
return userAuthFunctions.mergeAuthorizations(userAuthFunctions.getRequestedAuthorizations(requestedAuthorizations, currentUser.getPrimaryUser()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second call to currentUser.getPrimaryUser() could use primaryUser variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. fixing in next commit.
public List<Key> getKeys(String modelTable, DatawaveUserDetails currentUser, String regexTerm) throws TableNotFoundException { | ||
Scanner scanner = ScannerHelper.createScanner(this.accumuloClient, modelTable, this.getAuths(currentUser)); | ||
if (!regexTerm.isEmpty()) { | ||
IteratorSetting cfg = new IteratorSetting(21, "colfRegex", RegExFilter.class.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how many of the calls to this require a regex and if they can be replaced with an exact colf value which could be more performant, depending on locality groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is representative of the original logic here: https://github.com/NationalSecurityAgency/datawave/blob/integration/web-services/model/src/main/java/datawave/webservice/query/model/ModelBean.java#L356
This was part of some refactor work Ceria did to move the model rest endpoint to the dictionary service.
The base branch was changed.
No description provided.