Skip to content

Conversation

@Krish-cloudsufi
Copy link
Owner

Incorporated SRV format in connection string and introduced a toggle property in MongoDB plugin UI.

pom.xml Outdated
<cdapArtifacts>
<parent>system:cdap-data-pipeline[6.8.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent>
<parent>system:cdap-data-streams[6.8.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent>
<parent>system:cdap-data-pipeline[6.11.0-SNAPSHOT,7.0.0-SNAPSHOT)</parent>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

pom.xml Outdated
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<!-- version properties -->
<cdap.version>6.8.0-SNAPSHOT</cdap.version>
<cdap.version>6.11.0-SNAPSHOT</cdap.version>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this change

@Name(MongoDBConstants.USE_SRV)
@Description("Enable SRV format for connection string")
@Nullable
private Boolean useSRV;
Copy link

@vikasrathee-cs vikasrathee-cs Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it primitive at all places

if (!containsMacro(MongoDBConstants.HOST) && Strings.isNullOrEmpty(host)) {
throw new InvalidConfigPropertyException("Host must be specified", MongoDBConstants.HOST);
}
if (!containsMacro(MongoDBConstants.PORT)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this condition as when useSRV is not macro and not true, this validation is required

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

private String password;

@Name(MongoDBConstants.USE_SRV)
@Description("Enable SRV format for connection string")
Copy link

@vikasrathee-cs vikasrathee-cs Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Connect Using SRV String as field and label name in widget and also add the same in md doc file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add same description that you have added in md file

@Name(MongoDBConstants.USE_SRV)
@Description("Enable SRV format for connection string")
@Nullable
private boolean useSRV;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change field name also

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall I make it as connectUsingSRV ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name should be same as given in widget.json

return password;
}

public boolean isUseSRV() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding 'is' as a prefix doesnt make much sense, keep it just connectUsingSRVString


return connectionStringBuilder.toString();


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove blank lines

public static final String PASSWORD = "password";

/**
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a small description

public MongoDBSinkConfig(String referenceName, String host, int port, String database, String collection,
String user, String password, String connectionArguments, String idField) {
super(referenceName, host, port, database, collection, user, password, connectionArguments);
String user, String password, Boolean useSRV, String connectionArguments, String idField) {
Copy link

@vikasrathee-cs vikasrathee-cs Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make boolean primitive at all places


**Password:** Password to use to connect to the specified database.

**Connect Using SRV String:** Toggle to determine whether to use an SRV connection string for MongoDB. It can be

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add same to sink also

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

/**
* Configuration property name used to specify whether to use an SRV Connection string for MongoDB.
*/
public static final String USE_SRV = "connectUsingSRVString";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this also to CONNECT_USING_SRV_STRING

@Description("Toggle to determine whether to use an SRV connection string for MongoDB. It can be " +
"enabled if the MongoDB deployment supports SRV DNS records for connection resolution.")
@Nullable
private boolean connectUsingSRVString;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this condition below to check this plugin property whether it contains macro or not, but the plugin property is not annotated with @macro annotation.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

protected String collection;
protected String user;
protected String password;
protected Boolean connectUsingSRVString;
Copy link
Collaborator

@sgarg-CS sgarg-CS Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to boolean ? as the main config file uses the primitive type for the same variable.
Boolean can cause NPE, if the variable is not set.

@sgarg-CS
Copy link
Collaborator

LGTM.

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 this pull request may close these issues.

4 participants