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

CreateCollection silently drops supported options #79

Closed
jsonking opened this issue Jan 16, 2021 · 3 comments · Fixed by #78
Closed

CreateCollection silently drops supported options #79

jsonking opened this issue Jan 16, 2021 · 3 comments · Fixed by #78

Comments

@jsonking
Copy link

jsonking commented Jan 16, 2021

The CreateCollectionChange silently drops valid options allowed by the server (only retaining the validation related ones if defined).
This is a potential source of error as the user may not notice this when defining and running a changeset as the collection would be created without the options being present.

I have raised a PR for fixing this: #78

In the PR I have changed the implementation to issue a runCommand operation instead. This allows all supported database options to be defined in the changeset which are then passed through to the server. This is more future proof than using the driver's CreateCollectionOptions class which evolves over time and requires manually copying each option across.

If you like this approach I am happy to help convert other similar methods to do the same (e.g. createIndex which recently had changes for a similar reason in Issue-74).

┆Issue is synchronized with this Jira Bug by Unito

@alexandru-slobodcicov
Copy link

Thank you @jsonking for you suggestion and PR. Please note I have add some cosmetic changes to you PR and merged

@jsonking
Copy link
Author

jsonking commented Jan 24, 2021

Hi - Thanks @alexandru-slobodcicov - glad to have helped.
I'm happy to help convert other Statements to us the same pattern: which will provide support for all options now and reduce future maintainence. A side effect is that the library should then work with different driver versions as desired by users - so perhaps we could start with statements that the library directly uses first?

Would appreciate if you could explain the role of the toJs methods?
In the PR I changed AbstractCollectionStatement to make this abstract so that implementers have to provide a meaningful implementation:

    /**
     * Provides a javascript representation of the command
     *   (for example that can be ran in the mongo shell).
     * @return javascript version of the full command
     */
    @Override
    public abstract String toJs();

Noticed that in the merged PR this class provides a default implementation:

    /**
     * Provides a pseudo javascript representation of the collection related statement
     *   (for example that can be ran in the mongo shell.
     *   Exceptions examples are count which uses db.getCollectionNames however filters programmatically by name).
     * @return javascript version of the full command
     */
    @Override
    public String toJs() {
        return "db." +
                getCommandName() +
                "(" +
                getCollectionName() +
                ");";
    }

The javadoc comment now mentions an exception (count); which people are unlikely to read if they are looking at the CountCollectionByNameStatement class file or update when making changes elsewhere.

i.e. feels like this should indeed be abstract? else I am mis-understanding (not for the first time).

Thanks
Jason

@alexandru-slobodcicov
Copy link

Hi @jsonking toJs is coming somewhere in DATABASECHANGELOG collection, suppose in description field. Don't think it shuld be that accurate and not critical. If you could really help with converting the possible Statements to the runCommand approach would help a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants