Feature Request: Configuration setting that throws error on duplicate registration

[brianasapp]

serialijse/lib/serialijse.js

Line 105 in 150921d

console.warn("declarePersistable warning: declarePersistable : class " + className + " already registered");

On the above line - the user is warned that their original constructor will be overrided, but there is no way to prevent this via configuration.

This becomes dangerous in a larger codebase, when the probability for naming collisions increases. Consider the following example

src/login/form.ts

export class LoginForm {
    constructor() {
        // renders a simple login form
    }
}

declarePersistable(LoginForm);

src/oauth/form.ts

export class LoginForm {
    constructor() {
        // renders an oauth confirmation screen
    }
}

declarePersistable(LoginForm);
// this will overwrite the original LoginForm

Now the simple solution is just to "not do this", but as the number of persistable objects grows - the chances for this type of collision increases as well. Codebases which have additional log statements might mask the severity of this mistake.

My suggestion is that we use some environment variable - perhaps SERIALIJSE_SAFE_MODE to fail loudly anytime the described situation occurs. I would happily open a pull request if we are in agreement.

Thanks for your time,
Brian

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[erossignon]

There is also an other situation that can lead to this error. This may happen when you have a large projet with many modules depending on a a module which as been declared using 2 differents versions. This may happen if version increase has not been done properly.

For instance:

+ package.jon
+  node_modules
       + m1
            + foo@0.0.1
       + m2
            + foo@0.0.2
 

// module foo
class Foo {
};
declarePersistable(Foo);

In this case , there will be two instances of the foo.js module in the code and declarePersistable(Foo); will be called twice.

[brianasapp]

That makes sense, but I would argue you can reconcile the described scenario by doing the following.

class Foo {
};
declarePersistable(Foo, 'foo@0.0.1');

class Foo {
};
declarePersistable(Foo, 'foo@0.0.2');

My ultimate goal is that my team members would not be able to commit code that overwrites someone else's serialized class (which I believe will happen frequently given the nature of our work). We could use a verbose name parameter as I above suggested, but that currently would not stop the build from passing if there were a collision.

The reason I ask for this feature is in order to prevent overrides, I'll need to write a bash test that instantiates my application and greps for the already registered message. I would feel much more comfortable if I could set an environment variable that throws a proper javascript error, and modified my npm start command to set the proper variable.

Thanks again at any rate for the consideration,
Brian

[brianasapp]

Apologies, I totally misread your last message!

May I go ahead and open the PR? I have another proposed solution which involves attaching a hash value to the class prototype and using that value to serialize and deserialize the objects. Open to either approach, and sorry for the misunderstanding

Best,
Brian

[erossignon]

Hi Brian, I was about to suggest using adding an optional vendor hash to the declarePersistable,
The vendor hash could then be mended to the class name
as as you noticed , you might want to be warn if the same package is included twice with different package version. I agree with your proposed solution , please go ahead,
Do not forget to provide comprehensive unit tests , and make sure that we keep backward compatibility to ensure that already persisted json stuff can be reloaded without problem.

[brianasapp]

That's a great idea, thank you for the suggestions and your responsiveness. Will try to get to this in the near future, and will try my best to make the tests comprehensive as well as the code clear.

[brianasapp]

Hi Etienne,

Had a couple of comments I would like to run by you before I continue. First I'd like to check that my approach for generating a vendor hash is okay. I wasn't sure if it was the correct approach, so any feedback is more than welcome. It is as follows -

1. Retrieve and sort a list of the function names on both the constructor and the prototype (for static and instance functions).
2. Use the built in node toString function on each of the aforementioned functions, appended to their function names.
3. Generate a sha256 hash of the output string and place it directly on the constructor in the field constructor.SerialijseHashValue.

Lastly, I have thought of three immediate options for using the new approach in addition to maintaining backwards compatibility.

1. Creating a new function declarePersistableLock on the exports.
2. Adding a new g_classInfosLock object which holds the locked values.
3. Setting the original key (just the className) on g_classInfos as well as the className + vendor hash key.

Sorry to keep the back and forth going, just want to ensure I'm working in the correct direction.

Best,
Brian