davidzollo commented on PR #10447:
URL: https://github.com/apache/seatunnel/pull/10447#issuecomment-3904509629
The structure looks promising, but there are some issues that will cause
runtime failures and need to be addressed before merging.
## Critical Issues
### 1. `HubSpotSourceFactory` is missing `createSource` implementation
**File:**
`seatunnel-connectors-v2/connector-http/connector-http-hubspot/src/main/java/org/apache/seatunnel/connectors/seatunnel/hubspot/source/HubSpotSourceFactory.java`
The `TableSourceFactory` interface provides a default implementation for
`createSource` that throws an `UnsupportedOperationException`. Since you
haven't overridden it, the SeaTunnel engine will crash when trying to create
this source via the factory.
**Required Change:**
Please implement the `createSource` method:
```java
@Override
public <T, SplitT extends SourceSplit, StateT extends Serializable>
TableSource<T, SplitT, StateT> createSource(TableSourceFactoryContext context) {
return () -> (SeaTunnelSource<T, SplitT, StateT>) new
HubSpotSource(context.getOptions());
}
```
### 2. Unsafe Reflection Usage
**File:**
`seatunnel-connectors-v2/connector-http/connector-http-hubspot/src/main/java/org/apache/seatunnel/connectors/seatunnel/hubspot/source/HubSpotSourceParameter.java`
Using reflection to modify the private field `contentField` of the parent
class `HttpParameter` is highly discouraged ("Anti-pattern"). It makes the code
fragile and dependent on the internal implementation of the parent class.
```java
// Please REMOVE this:
Field contentFieldVar = HttpParameter.class.getDeclaredField("contentField");
contentFieldVar.setAccessible(true);
contentFieldVar.set(parameter, DEFAULT_CONTENT_FIELD);
```
**Suggestion:**
Instead of hacking the parent class via reflection, you should ensure the
configuration passed to the `HubSpotSource` constructor contains the default
value if the user hasn't provided one.
In `HubSpotSourceFactory.createSource`, you can merge the user options with
your default options:
```java
// Pseudocode example for Factory
ReadonlyConfig options = context.getOptions();
Map<String, Object> configMap = new HashMap<>(options.toMap());
if (!configMap.containsKey("content_field")) {
configMap.put("content_field", "$.results");
}
// Create source with modified config
return () -> (SeaTunnelSource<T, SplitT, StateT>) new
HubSpotSource(ReadonlyConfig.fromMap(configMap));
```
## Improvements & Cleanup
### 1. Clean up Debug/Hack Comments
**File:**
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-http-hubspot-e2e/src/test/java/org/apache/seatunnel/e2e/connector/hubspot/HubSpotIT.java`
**File:**
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-http-hubspot-e2e/src/test/resources/hubspot_source_case.conf`
Please remove comments like `CRITICAL FIX: ...` or `# STRICT CONFIGURATION
TO FIX EXIT CODE 1`. The code should stand on its own without artifacts from
your debugging session.
### 2. Constructor Logic
**File:** `HubSpotSource.java`
The logic inside the constructor is trying to compensate for the missing
configuration:
```java
if (this.contentField == null) {
this.contentField = HubSpotSourceParameter.DEFAULT_CONTENT_FIELD;
}
```
If you fix the parameter injection in the **Factory** (as suggested above),
this null check becomes unnecessary because `HttpSource` (the parent) will have
already initialized `this.contentField` correctly during its own constructor
execution using the config you passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
