NEOS-1379: handle duplicate mongo mappings #2581
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
evisdrenova
changed the title
evisdrenova
force-pushed
the
duplicateMongoMappings
branch
from
90cad2d to
30f1d55
Compare
There was a problem hiding this comment.
Left a few questions...some things I don't quite understand maybe you could help with.
Also, it would be a good sanity check if you could also make sure this stuff all works with Dynamodb.
Some of the string slicing is a little scary to me and would just be nice tomake sure it all is working for both mongo and dynamo.
| // we pass in an optional curr value so that we don't throw errors when searching for duplicates and the existing key is there | ||
| // for ex. if the initial value is "email" and we change it to "emaila" and then back to "email", it throws an error because the key set is made from all of the rows of the data which includes the original value, so we delete it if currValue is valid so we don't throw that error and allow the user to just save their current value without changing it | ||
| const isDuplicateKey = useCallback( | ||
| (newValue: string, schema: string, table: string, currValue?: string) => { |
There was a problem hiding this comment.
This doesn't seem right to me.
I would expect a function called isDuplicateKey to be a signature similar to: function isDuplicateKey(key: string): boolean
or at most: isDuplicateKey(schema: string, table: string, key: string): string
So I'm pretty confused as to why there is a currValue and newValue here.
There was a problem hiding this comment.
My thinking here is that if a user is trying to edit a mapping in line, then we have a newValue that we have to use as a key to check against the existing data to see if it's there. That value changes on every keystroke and it's not saved to the data object yet.
So we use the newValue to create a string that we can then check to see if it exists in the set. If it does, then we return true for the duplicate.
The currValue is optional here since it's only used for the inline mapping and is used to ensure that an existing mapping doesn't throw a value.
If the data object is: [{a.b.c},{d.e.f}], and the user clicks on the edit button, if we don't know what the currValue of the row that the user is trying to edit, then we throw an error if the user wants to not make any changes. For. ex. if the current row mapping is {a.b.c} and we want to edit this, the user makes a change to {a.b.t}, that works since that's not a mapping that exists. However, if the user, for whatever reason doesn't want to make the change and just wants to exist the mapping, then they will revert the mapping to {a.b.c} which will throw a duplicate error since that already exists.
So the currValue is used to prevent this case by deleting the mapping that the user is currently editing from the set so that it doesn't throw a duplicate error and the user can save the original mapping.
The other way we can fix this is to give the user a button to exit the editing process which will then end the editing process and not save the update. I didn't do that since it felt like too many buttons already in that small form.
There was a problem hiding this comment.
I see your thinking.
However, this is really just an edge case when the newValue and currValue are equivalent for the existing collection right?
onDuplicate should be smart enough to know that for the current rowIndex being updated, if the values are the same, don't do anything.
onDuplicate could just not fire if it knows curValue === newValue.
Anyways, just a thought, maybe I'm missing something here, but my spidey senses tend to go off when I see mutations in functions that are just checking if something exists in a set.
There was a problem hiding this comment.
will handle this in a new PR and clean it up a little more
No description provided.