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

Why did using a simple computed property name doesn't map properly? #289

Closed
micalevisk opened this issue May 3, 2021 · 8 comments · Fixed by #293
Closed

Why did using a simple computed property name doesn't map properly? #289

micalevisk opened this issue May 3, 2021 · 8 comments · Fixed by #293
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@micalevisk
Copy link
Contributor

micalevisk commented May 3, 2021

Hi!

Basically this, for instance:

      mapper
        .createMap(User, UserVm)
        .forMember(
          (d) => d.first, // works ✅
          mapFrom((s) => s.firstName)
        )
//
      mapper
        .createMap(User, UserVm)
        .forMember(
          (d) => d['first'], // this does not ❌
          mapFrom((s) => s.firstName)
        )

the later will result in an empty string as prop key ''

I'm trying to create a mapping to "en-us" property name. I can circumvent the usage of manual mapping by adding a getter method get ['en-us'](){ return this['en-us'] } in the source class, and with SnakeCaseNamingConvention naming convention.


  • @automapper/*: v3.5.2
@nartc
Copy link
Owner

nartc commented May 3, 2021

export function getMemberPath(fn: Selector): string {
this is a limitation atm. Feel free to poke around with getMemberPathUtil to see if you can come up with a better implementation to support that property.

@nartc nartc added enhancement New feature or request help wanted Extra attention is needed labels May 3, 2021
@micalevisk
Copy link
Contributor Author

micalevisk commented May 3, 2021

oh I saw this behavior documented here

path = getMemberPath(function (s: Foo) {
return s['odd-property'];
});
expect(path).toEqual('');

@micalevisk
Copy link
Contributor Author

micalevisk commented May 4, 2021

@nartc I'm thinking of supporting the following usages:

  • for completeness:
    • d => d['someThing'] and d => d['snake_prop'] (just like TypeORM does allow) But not the real computed property name ones like d => d['some' + 'Thing'] because I think those usages are pretty complex to handle properly ATM (I didn't understand how TypeORM support this but I'll probably take a look at it)
    • nested mixed ones like d => d['odd-prop'].foo and d => d.foo['odd-prop']
      Because currently d => d['bar'].baz is returning "baz"
      And d => d.bar['baz'] is returning "bar['baz']"
      while d => d.bar.baz returns "bar.baz"
  • d => d['odd-prop'] to avoid polluting the source class with a bunch of getter methods. Thus fixing the odd-property name limitation

Is it worth it or nah? (note that I'm not sure if I'll succeed implementing all of above but I can try)

@nartc
Copy link
Owner

nartc commented May 4, 2021

@micalevisk That works for me. One question on the 2nd bullet there: what would be your expected return value for d => d.bar.baz?

@micalevisk
Copy link
Contributor Author

micalevisk commented May 4, 2021

One question on the 2nd bullet there: what would be your expected return value for d => d.bar.baz?

I expect "bar.baz"

which is already done

const path = getMemberPath(function (s: Foo) {
return s.bar.baz;
});
expect(path).toEqual('bar.baz');

@nartc
Copy link
Owner

nartc commented May 4, 2021

Cool. Just want to confirm. Feel free to submit the PR anytime, I'll review it.

@micalevisk
Copy link
Contributor Author

micalevisk commented May 9, 2021

I'm trying one solution locally. This is what I've got so far:

  • for now, I'll treat only arrow functions for fn: Selector
  • to the output of cleanseAssertionOperators I'll apply this regular expression (or /(?<=\.)[^.]+/g if it has no square brackets) to grab all prop names as an array of strings and them join with . to produces the output of getMemberPath
  • I'll try my best to optimise space and time to not penalize the users that doesn't care about that limitation
my test suite
// all bellow are passing ✅

describe('cases that are allowed', () => {
  const cases = [
   // ____ input ________  __ output __
   , [ s=>s.foo        , ['foo'] ]
   , [ s=>s.foo.bar    , ['foo', 'bar'] ]
   , [ s=>s.returnFoo  , ['returnFoo'] ]
   , [ s=>s.return_foo , ['return_foo'] ]
  
   , [ s=>s[' aaa '] , [' aaa '] ]
   , [ s=>s['aaa']   , ['aaa'] ]
   , [ s=>s['ab-cd'] , ['ab-cd'] ]
   , [ s=>s['ab_cd'] , ['ab_cd'] ]

   , [ s=>s['bbb'].bar , ['bbb', 'bar'] ]
   , [ s=>s.bbb['bar'] , ['bbb', 'bar'] ]

   , [ s=>s['aaa']['ddd']     , ['aaa', 'ddd'] ]
   , [ s=>s['aaa'].ccc['ddd'] , ['aaa', 'ccc', 'ddd'] ]

   , [ s=>s['.foo.bar']     , ['.foo.bar'] ]
   , [ s=>s['foo=>s[bar']   , ['foo=>s[bar'] ]
   , [ s=>s["['a']"]        , ["['a']"] ]
   , [ s=>s['bad[foo]']     , ['bad[foo]'] ]
   , [ s=>s.bar['bad[foo]'] , ['bar', 'bad[foo]'] ]
   , [ s=>s["'a"]           , ["'a"] ]
   , [ s=>s['aa']['bo"o']   , ['aa', 'bo\"o'] ]

   , [ s=>s['with_sṕéçiâl_chàrs'] , ['with_sṕéçiâl_chàrs'] ]

   , [ s=>s['á']      , ['á'] ]
   , [ s=>s.á         , ['á'] ]
   , [ s=>s.good.á    , ['good', 'á'] ]
   , [ s=>s['good'].á , ['good', 'á'] ]
   , [ s=>s.á['good'] , ['á', 'good']

   , [ s=>s['']     , [''] ]
   , [ s=>s.foo[''] , ['foo', ''] ]
   , [ s=>s[''].foo , ['', 'foo'] ]
  ].map(([fn, expected]) => [fn.toString(), expected])

  it('should return the expected path list', () => {
    cases.forEach(( [fnSelector, expected] ) => {
      assert.deepEqual(getMembers(fnSelector), expected)
    })
  })
})

describe('cases that are disallowed', () => {
  const cases = [
   , [ s=>s      , null ] // Not a real one tho
   , [ s=>s`foo` , null ]

   // Known limitations that should be avoided in user land code because
   // they will produce wrong outputs and cannot be detected beforehand
   , [ s=>s['fo' + 'o']           , ['fo'] ] // expected to be ['foo']
   , [ s=>s[true ? 'foo' : 'bar'] , null ]   // expected to be ['foo']
   , [ s=>s[true && 'foo']        , null ]   // expected to be ['foo']

   , [ s=>s[Symbol()] , null ] // I'm not sure if we should support this
   , [ s=>s[`a`]      , null ] // To discourage the use of computed names
  ].map(([fn, expected]) => [fn.toString(), expected])

  it('should return the expected output', () => {
    cases.forEach(( [fnSelector, expected] ) => {
      assert.deepEqual(getMembers(fnSelector), expected)
    })
  })
})
my WIP solution
/**
 * (not the final solution but it works)
 * @param {string} fnSelector A serialized and clean arrow function expression.
 * @returns {string[]|null}
 */
function getMembers(fnSelector) {
  // maybe I'll remove this altogether since from `const re2 = ...` bellow we already generated the right output
  const RE_HAS_COMPUTED_PROP = /(^\w+=>\w+\[)|(]$)/;
  if (!RE_HAS_COMPUTED_PROP.test(fnSelector)) {
    const re1 = /(?<=\.)[^.]+/g;
    return fnSelector.match(re1);
  }

  const re2 = /(?:(?<=\[)(['"]).*?\1)|(?:(?<=\.)[^.\[]+)/g;
  const matches = fnSelector.match(re2);
  // May be we could optimize this by using `re2.exec`
  // instead to prevent doing `.map` and `.match` and `.slice`
  if (matches) {
    const RE_HAS_QUOTES = /^(['"]).*\1$/;
    return matches.map(match =>
      match.match(RE_HAS_QUOTES)
          ? match.slice(1, -1) // drop the first and the last quotes
          : match
    );
  }
  return null;
}

/*

// Explanation of RE_HAS_COMPUTED_PROP regex

(
  ^     # from the beginning of the string
  \w+   # followed by 1 or more word character (alphanumeric ASCII and underscore)
  =>    # followed by the literal "=>"
  \w+
  [     # followed by the literal "["
|       # Or matches with...
  \]    # any occurrence of the literal "]" that is
  $     # in the end of the string
)


// Explanation of re1 regex

(?<=   # (begin of positive lookbehind)
  \.   # matches a literal "." char but without including it in the match result
)      # (end of positive lookbehind)
[^.]+  # followed by 1 or more occurrence of any char but "." (dot)


// Explanation of re2 regex

(?:         # (begin of non-capturing group)
  (?<=      # (begin of positive lookbehind)
    \[      # matches a  literal "[" but without including it in the match result
  )         # (end of positive lookbehind)
  (         # (begin capturing group #1)
    ['"]    # match one occurrence of a single or double quotes characters
  )         # (end capturing group #1)
  .*?       # followed by 0 or more of any character, but match as few characters as possible (which is 0)
  \1        # followed by the result of capture group #1
)           # (end of non-capturing group)
|           # Or matches with...
(?:         # (begin of non-capturing group)
  (?<=      # (begin of positive lookbehind)
    \.      # matches a literal "." but without including it in the match result
  )         # (end of positive lookbehind)
  [^.\[]+   # followed by 1 or more occurrences of any character but "." nor "["
)           # (end of non-capturing group)


// Explanation of RE_HAS_COMPUTED_PROP regex

^       # from the beginning of the string
(       # (begin capturing group #1)
  ['"]  # match one occurrence of a single or double quotes characters
)       # (end of capturing group #1)
.*      # followed by 0 or more occurrences of any character
\1      # followed by the result of capture group #1
$       # in the end of the string

*/

the last regex

@nartc any thoughts about this approach?


Note that the output of ['foo', ''].join('.') will be 'foo.', and I follow the usage of this bellow (memberPath variable)

const mappingProperty: MappingProperty = [
[memberPath, sourcePath],

(memberPath, nestedDestination) => (value) => {
nestedDestination = set(nestedDestination, memberPath, value);

and I've seen that set({}, 'foo.', 123) returns { foo: { '': 123 } } which is the desired output ^^ but I'll add this test cases in set.spec.ts

@micalevisk
Copy link
Contributor Author

micalevisk commented May 11, 2021

@nartc nvm I just open the PR #293 ^^ feel free to change anything

nartc pushed a commit that referenced this issue May 18, 2021
* test(core): add empty string and trailing dot paths cases

* feat(core): add a partial support for computed property names

Computed property names were introduced in ES6.

"Partial" in a sense that few computed property name usage are not
supported. As well for when using with ES5 functions, not arrow
functions.

Closes #289

* docs: adjust odd-property name limitations

* feat(core): improve `getMembersFromArrowFunctionExpr` to avoid map and match all the away

* feat(core): add suport to computed property names for ES5

* docs: remove the ES5 limitation of odd-property name

* feat(core): reset the state of the shared regex before

Since we are using a constant regex that has the `g` flag in iterator
we should make sure that it is in the initial state.
See http://speakingjs.com/es5/ch19.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants