Making user work #250
Making user work #250
Conversation
Trying to fix the simple user entity type
Another atempt
…'t feel great about it though. Need to clean up some.
bvsharp
force-pushed
the
making-user-work
branch
from
1231ec5
to
651d49f
Compare
kjain110
force-pushed
the
making-user-work
branch
from
651d49f
to
c23063c
Compare
bvsharp
force-pushed
the
making-user-work
branch
from
c23063c
to
35c91f3
Compare
kjain110
force-pushed
the
making-user-work
branch
from
35c91f3
to
7807307
Compare
| return Optional.of(entityType); | ||
| } | ||
|
|
||
| private void buildCompositeEntityTypeDefinition(EntityType entityType) { |
There was a problem hiding this comment.
Since the code above is commented out, this method is unused
| return Optional.of(entityType); | ||
| } | ||
|
|
||
| private void buildCompositeEntityTypeDefinition(EntityType entityType) { |
There was a problem hiding this comment.
This method's name isn't very clear to me. When I see build_____(), I'd normally expect the method to return something. When I see that name but it's void, I have to go read the code to understand what it does
| return Optional.of(entityType); | ||
| } | ||
|
|
||
| private void buildCompositeEntityTypeDefinition(EntityType entityType) { |
There was a problem hiding this comment.
I could be wrong, but at a quick glance, it looks like this method can/should be static
| } | ||
|
|
||
| private void buildCompositeEntityTypeDefinition(EntityType entityType) { | ||
| System.out.println("BUILD COMPOSITE DEFINITION"); |
There was a problem hiding this comment.
These System.out.println() calls should be replaced with calls to the logger
| System.out.println("BUILD COMPOSITE DEFINITION"); | ||
| List<EntityTypeSourceJoin> entityTypeSourceJoins = new ArrayList<>(); | ||
| List<EntityTypeSource> sources = entityType.getSources(); | ||
| entityType.setFromClause("BUILD COMPOSITE DEFINITION"); |
There was a problem hiding this comment.
Personal opinion: we should avoid using the fromClause if we're not actually using it for real, so that we can remove it from the object model ASAP. The more we use it, the harder it will be to remove
| EntityTypeSourceJoin join = source.getJoin(); | ||
| String joinType = join.getType(); | ||
| String joinCondition = join.getCondition(); |
There was a problem hiding this comment.
Just stating the obvious here since this is implied by the comment above, but if source.getJoin() is null, we'll get an NPE here (and NPEs are always bugs)
| if (i == 0) { // TODO: think about replacing this with -- if (source.getJoin() == null) | ||
| fromClause.append(source.getAlias()); | ||
| } else { | ||
| if (source.getType().equals("db")) { |
There was a problem hiding this comment.
| if (source.getType().equals("db")) { | |
| if ("db".equals(source.getType())) { |
This way, we handles nulls and avoid a potential NPE
| @@ -63,12 +69,12 @@ public int streamIdsInBatch(UUID queryId, | |||
| Consumer<IdsWithCancelCallback> idsConsumer) { | |||
| UUID entityTypeId = queryDetailsRepository.getEntityTypeId(queryId); | |||
| EntityType entityType = getEntityType(entityTypeId); | |||
| Condition condition = field(ID_FIELD_NAME).in( | |||
| Condition condition = field("\"source1\".id").in( | |||
There was a problem hiding this comment.
Don't forget to do something with this string before we merge this :)
| String finalWhereClause = sqlWhereClause.toString(); | ||
| for (EntityTypeSource source : entityType.getSources()) { | ||
| String toReplace = ":" + source.getAlias(); | ||
| String alias = "\"" + source.getAlias() + "\""; | ||
| finalWhereClause = finalWhereClause.replace(toReplace, alias); | ||
| } |
There was a problem hiding this comment.
We should do this replacement by using SQL parameters, if at all possible. Doing it manually is generally pretty dangerous and bug-prone. The code for handling valueFunction does it that way, so it could be a useful reference
| @@ -104,11 +109,28 @@ public List<Map<String, Object>> getResultSet(UUID entityTypeId, Fql fql, List<S | |||
| } | |||
|
|
|||
| Condition condition = FqlToSqlConverterService.getSqlCondition(fql.fqlCondition(), entityType); | |||
| // TODO: might want to put next 5 lines in its own method in EntityTypeFlatteningService | |||
There was a problem hiding this comment.
Agree 100%, especially since this same code shows up in IdStreamer
| .stream() | ||
| .filter(EntityTypeColumn::getIsIdColumn) | ||
| .findFirst() | ||
| .orElseThrow() |
There was a problem hiding this comment.
This should throw a specific exception, or at least a generic one with a clear error message
| .name(originalEntityType.getName()) | ||
| ._private(originalEntityType.getPrivate()) | ||
| .defaultSort(originalEntityType.getDefaultSort()) | ||
| .columns(originalEntityType.getColumns()) // TODO: probably remove |
There was a problem hiding this comment.
Personal opinion: don't remove it. If we keep the columns, it makes it possible to augment an existing entity type by importing it and adding some fields
| Pair<EntityTypeSource, List<EntityTypeColumn>> updatePair = getConvertedSourceAndColumns(originalEntityType, source, null, false); // TODO: think about this, may not be able to hardcode false here | ||
| flattenedEntityType.addSourcesItem(updatePair.component1()); | ||
| finalColumns.addAll(updatePair.component2()); | ||
| } else { |
There was a problem hiding this comment.
I'd probably check for "entity-type" here and throw an exception when it's not one of the valid values.
| package org.folio.fqm.service; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import kotlin.Pair; |
There was a problem hiding this comment.
We should probably use a different implementation of Pair. We're not actually using Kotlin (unfortunately), so it just feels weird to use stuff from its standard lib
| List<EntityTypeColumn> finalColumns = new ArrayList<>(); | ||
| for (EntityTypeSource source : originalEntityType.getSources()) { | ||
| if (source.getType().equals("db")) { | ||
| Pair<EntityTypeSource, List<EntityTypeColumn>> updatePair = getConvertedSourceAndColumns(originalEntityType, source, null, false); // TODO: think about this, may not be able to hardcode false here |
There was a problem hiding this comment.
Comments like this make me wish we were using something where you can name parameters at the call-site, like kotlin. I have no idea what the false does here, which is something that could be fixed at the language-level if Java supported it
| // If an entity type source contains multiple db sources, then we need to keep the original alias in order to | ||
| // distinguish the different targets. Frequently, it will likely only have one db source. In this case we | ||
| // can use the outer alias only, in order to keep field names more concise | ||
| boolean keepOriginalAlias = countDbSources(flattenedSourceDefinition) > 1; |
There was a problem hiding this comment.
Personal opinion, but I could probably be swayed pretty easily: don't bother with this. I'd prefer less concise field names if it means simpler code and more consistent behavior.
| return Optional.of(flattenedEntityType); | ||
| } | ||
|
|
||
| public String getJoinClause(EntityType flattenedEntityType) { |
There was a problem hiding this comment.
Nit: I don't like this name, since afaik, there's no such thing as a join clause
| return orderedList; | ||
| } | ||
|
|
||
| private static void dfs(EntityTypeSource source, Map<String, EntityTypeSource> sourceMap, Set<String> visited, List<EntityTypeSource> orderedList) { |
There was a problem hiding this comment.
Not a fan of this method name, personally. I had to actually read the code to understand the name
| orderedList.add(source); | ||
| } | ||
|
|
||
| private void updateOtherSources(String oldSourceName, String newSourceName, List<EntityTypeSource> otherSources) { |
There was a problem hiding this comment.
This method name is killing me, too 😛 I dunno what "other sources" are, so it might as well be named doThings() 🙂
| return entityType | ||
| .getSources() | ||
| .stream() | ||
| .filter(source -> !Boolean.TRUE.equals(source.getFlattened()) && source.getType().equals("db")) |
There was a problem hiding this comment.
Maybe set the type to "flattened" or something and get rid of the flattened property? That'd simplify the object model a bit and probably simplify some code outside this method, too, too
| } | ||
| } | ||
|
|
||
| private long countDbSources(EntityType entityType) { |
There was a problem hiding this comment.
| private long countDbSources(EntityType entityType) { | |
| private static long countDbSources(EntityType entityType) { |
No description provided.