Dev - #32

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/prod_owlreka.yml:
- Around line 27-34: The workflow steps "Build with Maven" and "Upload artifact
for deployment job" are running from the repo root but the Maven project is
rooted at scipubtts; update both steps to run in that module: for the "Build
with Maven" step set the working-directory to the module (e.g.,
working-directory: scipubtts) and run mvn clean install there, and for the
"Upload artifact for deployment job" step set the same working-directory (or
adjust the artifact path) so path points to the module's target/*.jar (e.g.,
path: '${{ github.workspace }}/scipubtts/target/*.jar').
- Around line 21-25: The workflow uses actions/setup-java@v4 with an invalid
java-version value 'java25'; update the java-version input to a valid value
(e.g., '25' for GA or '25-ea' for Early Access) in the job step that uses
actions/setup-java so the action can resolve and install the correct JDK
distribution; keep distribution: 'microsoft' unchanged and ensure the step name
"Set up Java version" and the uses entry "actions/setup-java@v4" remain intact.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/auth/dto/request/RegisterLocalRequest.java`:
- Around line 33-34: The RegisterLocalRequest DTO exposes an unconstrained
String appBaseUrl which can enable open redirect/ token leakage when used to
construct verification links; add validation on the appBaseUrl field (e.g.,
require a well-formed absolute URI with scheme http/https, no
path/query/fragment, and max length) using javax.validation annotations or a
custom validator on RegisterLocalRequest.appBaseUrl and normalize it before use,
and enforce a server-side allowlist of permitted frontend origins (check the
normalized value against the allowlist in the registration flow where the
verification link is built) so only approved origins are accepted; update any
code that consumes RegisterLocalRequest.appBaseUrl to rely on the
validated/normalized value.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/auth/security/oauth2/OAuth2AuthenticationSuccessHandler.java`:
- Around line 38-42: Remove the duplicate field declaration of
AuthorizationRequestRepository<OAuth2AuthorizationRequest>
authorizationRequestRepository so there is only one field: initialize it with
new HttpSessionOAuth2AuthorizationRequestRepository() (keep that single
declaration) to fix the compilation/wiring issue. In the authentication success
flow (method handling OAuth2 success), remove the duplicate targetUrl variable
declaration (only declare it once) and modify the "new-user" (CASE 2) branch so
it does not re-check userRepository.findByEmail(email) and short-circuit;
instead, after generating the signup token, build the redirect URL to
"/register/complete?token=..." using the single targetUrl variable and perform
the redirect so the generated token is handed off to the client (ensure the
previously-generated token variable is used in the redirect). Ensure references
to authorizationRequestRepository and userRepository remain correct after these
changes.
- Around line 141-179: The method in OAuth2AuthenticationSuccessHandler declares
targetUrl twice and treats missing users as an error instead of redirecting them
to complete signup; fix by removing the duplicate String targetUrl declaration
and create a single targetUrl variable that is set conditionally: if
userRepository.findByEmail(email).isEmpty() then build the frontendBaseUrl +
"/register/complete" with the rawToken as a query param (and call
authorizationRequestRepository.removeAuthorizationRequest and redirect there),
otherwise call authSessionService.issueSession(userOptional.get(), false,
request, response) and set targetUrl to frontendBaseUrl + "/oauth2/success";
finally call getRedirectStrategy().sendRedirect(request, response, targetUrl).
Ensure you reference and use rawToken,
authorizationRequestRepository.removeAuthorizationRequest, redirectWithError
only where appropriate, and do not declare String targetUrl more than once.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/common/openalex/OpenAlexClient.java`:
- Around line 76-100: performGetWithRetry currently rethrows raw
RestClientResponseException/RestClientException on the final attempt, so the
terminal BusinessException(ErrorCode.OPENALEX_REQUEST_FAILED) is never reached;
change the two catch blocks in performGetWithRetry so that: (1)
RestClientResponseException still maps 404 to
BusinessException(ErrorCode.OPENALEX_ENTITY_NOT_FOUND); (2) if attempt ==
MAX_RETRIES do not rethrow the raw exception — instead throw new
BusinessException(ErrorCode.OPENALEX_REQUEST_FAILED) (optionally include or log
the original exception as cause); (3) otherwise keep the existing retry logic
(sleep/backoff) and call executeGet as before. Ensure function names referenced:
performGetWithRetry and executeGet.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/config/SecurityConfig.java`:
- Around line 47-48: The docsPublicEnabled flag is currently unused so
Swagger/OpenAPI endpoints are always permitted; update SecurityConfig to
conditionally allow or restrict docs based on docsPublicEnabled by replacing the
hard-coded antMatchers(...).permitAll() for the Swagger/OpenAPI paths with a
branch that, when docsPublicEnabled is true, calls antMatchers(...).permitAll(),
and when false either removes permitAll() or applies authenticated() (e.g.,
antMatchers(...).authenticated()), ensuring this logic is applied on the same
HttpSecurity authorization chain (the method that calls
authorizeRequests()/authorizeHttpRequests() in SecurityConfig). Use the existing
docsPublicEnabled field to drive the decision and keep the Swagger path list
unchanged.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/dashboard/service/MetricService.java`:
- Around line 77-80: The calculatePercentageChange method incorrectly returns
100.0 whenever previousValue == 0; update calculatePercentageChange(long
currentValue, long previousValue) to first check if previousValue == 0 and
currentValue == 0 and return 0.0 for that no-change case, then if previousValue
== 0 and currentValue > 0 return 100.0, otherwise fall back to the existing
percentage computation (using the normal (current - previous)/previous * 100
logic).

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/dto/SearchWorksQueryRequest.java`:
- Around line 104-108: The DTO SearchWorksQueryRequest must continue accepting
the legacy "sort" query parameter so callers using the old format don't lose
sorting: add a nullable String field named sort (with a deprecation note/Schema)
to SearchWorksQueryRequest and update the code path that resolves sort (in
SearchWorksLookupService or wherever sorting is parsed) to check for the legacy
sort value when sortBy/sortDirection are not provided—parse legacy values into
the same internal representation (e.g., "field" or "field:direction" -> sortBy
and sortDirection) so SearchController bindings keep working for both new and
legacy callers.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/dto/SearchWorksResponse.java`:
- Line 32: The rename of the SearchWorksResponse record component from topicName
to topic breaks the /api/search/works JSON contract (SearchController serializes
the record directly); restore compatibility by exposing the old JSON property
name while keeping the new internal name: either revert the record component to
topicName, or (preferred) add a compatibility accessor in SearchWorksResponse
such as a public topicName() method that returns topic (or annotate the existing
topic component with `@JsonProperty`("topicName")) so Jackson emits "topicName"
alongside/for the same value; update only the SearchWorksResponse record and
ensure serialization tests still pass.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/service/SearchFilterBuilder.java`:
- Around line 93-99: The current mapping in SearchFilterBuilder adds exclusive
OpenAlex operators for min/max which breaks inclusive DTO semantics; update the
logic that builds filterParts (the code that currently does
filterParts.add(field + ":>" + minValue) and filterParts.add(field + ":<" +
maxValue)) so that for publication_year (yearFrom/yearTo) you produce an
inclusive hyphen range string like "publication_year:min-max" when either bound
exists (handle open-ended bounds appropriately), and for cited_by_count
(citationMin/citationMax) emulate inclusive bounds by converting them to
exclusive operators with offsets (use "cited_by_count:>(min-1)" when citationMin
provided and "cited_by_count:<(max+1)" when citationMax provided); keep other
fields unchanged and ensure you reference the same filterParts collection and
field/name variables in SearchFilterBuilder.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/service/SearchOptionsService.java`:
- Line 30: The class-level Map field defaultFilterOptionsCache (type
CachedFilterOptions) is unbounded and can grow indefinitely from page-based
keys; replace it with a bounded, eviction-capable cache (e.g., Caffeine or Guava
Cache, or a size-limited synchronized LinkedHashMap) configured with a sensible
maximumSize and expireAfterWrite/Access policy. Update all places that currently
read/write defaultFilterOptionsCache (the code that composes keys like
"limit:page" and the methods that populate CachedFilterOptions) to use the
cache's get(key, mappingFunction) or getIfPresent/put pattern so entries are
computed on demand and evicted automatically; keep the CachedFilterOptions type
and population logic but move creation into the cache loader/mappingFunction.
Ensure thread-safety by using the cache API rather than manual ConcurrentHashMap
access so high-cardinality page values cannot grow memory without bound.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/service/SearchSummaryService.java`:
- Around line 29-40: getSummary currently only reads volatile cachedSummary and
on TTL expiry every concurrent caller will fetch from OpenAlex (cache stampede);
fix by introducing a dedicated final Object lock (e.g., summaryRefreshLock) and
use double-checked locking inside getSummary: after detecting currentSummary ==
null || currentSummary.isExpired(), synchronize(summaryRefreshLock) and re-check
cachedSummary/isExpired(), then only the thread that still needs to refresh
calls fetchTotalWorksCount(), creates new CachedSummary and assigns
cachedSummary; keep cachedSummary volatile and leave CachedSummary and
fetchTotalWorksCount unchanged.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/user/service/impl/AccountServiceImpl.java`:
- Around line 39-56: The code currently throws LOCAL_PASSWORD_NOT_AVAILABLE when
user.getPasswordHash() is blank, which prevents Google-only accounts from
setting their first local password; revert to a two-path flow in
AccountServiceImpl: if StringUtils.hasText(user.getPasswordHash())
(hasLocalPassword) keep the existing checks using
passwordEncoder.matches(request.currentPassword(), user.getPasswordHash()) and
throw CURRENT_PASSWORD_INVALID or PASSWORD_REUSE_NOT_ALLOWED as needed, but if
there is no local password allow a "first-password" path that does not require
currentPassword (validate request.newPassword() is present and not empty, and
ensure it isn't trivially invalid), then set the new password hash; remove the
unconditional throw BusinessException(ErrorCode.LOCAL_PASSWORD_NOT_AVAILABLE)
and restore the commented no-local-password branch (also update the same logic
at the other occurrence around lines 79-86) so Google-only users can establish
their initial credential.

---

Outside diff comments:
In @.github/workflows/prod_owlreka.yml:
- Around line 19-62: The workflow currently pins third-party actions by tag
(e.g., actions/checkout@v4, actions/setup-java@v4, actions/upload-artifact@v4,
actions/download-artifact@v4, azure/login@v2, azure/webapps-deploy@v3); replace
each tag reference with the corresponding full commit SHA for that action
(lookup the exact commit SHA from the action's GitHub repo and update the uses:
lines to e.g. actions/checkout@<full-sha>), verify the SHAs point to the
intended release commits, and commit those updated uses entries so the workflow
is pinned to immutable SHAs.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/dashboard/service/MetricService.java`:
- Around line 6-13: The file currently declares both an interface and a class
named MetricService which breaks compilation; keep this file as the interface
only by removing the concrete class declaration here and ensuring the interface
contains the method signature MetricsResponse
calculateAndSaveMetrics(PeriodRequest request). Move the implementation into a
new class named MetricServiceImpl (in the same package) that implements
MetricService, annotate it with `@Service` and `@RequiredArgsConstructor`, implement
the calculateAndSaveMetrics(PeriodRequest request) method body formerly in the
removed class, and add any necessary imports and constructor-injected
dependencies there; update any callers/beans to depend on the MetricService
interface (no other changes required in this file).

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/search/controller/SearchController.java`:
- Around line 155-203: The three mutating endpoints saveSearchHistory,
deleteSearchHistory, and clearSearchHistory should require an authenticated
user; change the `@CurrentUserUUID` annotations on these methods from required =
false back to required = true (or remove the parameter so it defaults to
required) so anonymous requests receive a 401 instead of reaching the service
with userId == null; update the annotations on the UUID parameter in
saveSearchHistory, deleteSearchHistory, and clearSearchHistory accordingly.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/admin/repository/AdminDashboardRepository.java`:
- Around line 206-213: The queryOptionalOffsetDateTime method uses
ZoneId.systemDefault() when converting the Timestamp to OffsetDateTime, which
causes the result to vary depending on the server's timezone, leading to
inconsistency across environments. Replace ZoneId.systemDefault() with a stable,
environment-independent timezone such as ZoneOffset.UTC or another fixed zone
identifier to ensure the same database value always produces the same
OffsetDateTime result regardless of where the code is executed.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/bookmark/entity/UserBookmark.java`:
- Around line 32-33: The entityType field in the UserBookmark class has a
default initializer of "WORK", but Lombok's `@Builder` annotation ignores field
initializers by default, causing the field to be null when constructed via the
builder. This violates the nullable = false constraint on the database column.
Add the `@Builder.Default` annotation directly above the entityType field
declaration to ensure the "WORK" default value is applied when using the builder
pattern.

In `@scipubtts/src/main/java/com/brotherhood/scipubtts/config/FlywayConfig.java`:
- Around line 26-27: The ignoreMigrationPatterns field is being injected from
Spring configuration properties at the class level, but the actual Flyway
configuration is using hardcoded values instead of this configured field,
causing the property to never be applied at runtime. Locate where the hardcoded
ignoreMigrationPatterns values are being set (likely in a method that configures
the Flyway bean) and replace those hardcoded values with the
ignoreMigrationPatterns field that already contains the configuration values
from the Spring properties file.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/config/OpenAlexConfig.java`:
- Around line 20-34: The RestClient builder configuration has three issues to
fix. First, remove the redundant first call to baseUrl() on line 21 that
hard-codes "https://api.openalex.org", keeping only the second call with the
configured openAlexBaseUrl variable. Second, remove the redundant first call to
defaultHeader() on line 22 that sets User-Agent to "ScipubTTS", keeping only the
second call that uses HttpHeaders.USER_AGENT constant. Third, fix the empty
api_key parameter in the requestInterceptor method where it currently appends
"api_key=" with an empty string value. Either inject the api_key from
application configuration using a `@Value` annotation on a field like
openAlexApiKey and insert that value into the URI construction, or remove the
entire requestInterceptor block if OpenAlex does not require authentication.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/feed/client/OpenAlexWorksClient.java`:
- Around line 47-55: The fetchWithRetry method in OpenAlexWorksClient only
catches RestClientResponseException for HTTP response errors, but transient
network failures like timeouts and connection resets throw RestClientException
instead and are not retried. Modify the exception handling to catch the parent
RestClientException class, and add logic to distinguish between
RestClientResponseException (which should retry on 429 or 5xx status codes) and
other RestClientException types (which should retry if attempt is less than 3).
This ensures both HTTP error responses and transport-level transient failures
are properly retried before giving up.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/feed/service/impl/ResearchFeedSyncServiceImpl.java`:
- Around line 52-65: The syncDailyFeed method in ResearchFeedSyncServiceImpl
lacks protection against concurrent execution, allowing overlapping runs from
the scheduler and manual triggers. Before creating and saving a new ApiJob with
STATUS_RUNNING, add a check to query the apiJobRepository for any existing jobs
with the same JOB_TYPE and STATUS_RUNNING status. If a running job already
exists, return early to prevent the concurrent execution. Only proceed with
creating and saving the new job if no other running sync is in progress.
- Around line 106-123: The method currently returns void, preventing callers
from knowing whether the sync operation succeeded, partially succeeded, or
failed. Modify the method to return the job status (or a result object
containing the outcome) instead of returning void. After setting the job status
and saving to the repository in the normal flow, return the status value. Also
ensure the catch block returns or rethrows appropriately so that callers can
access the actual outcome of the sync operation and handle partial successes or
failures accordingly.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/schedule/ResearchFeedScheduler.java`:
- Around line 14-16: The syncResearchFeedDaily() method's `@Scheduled` annotation
does not specify a timezone parameter, but the researchFeedSyncService uses
Asia/Ho_Chi_Minh timezone for its date window calculations. Add the zone
parameter to the `@Scheduled` annotation and set it to "Asia/Ho_Chi_Minh" to
ensure the cron job executes at the correct wall-clock time relative to the date
window logic used in ResearchFeedSyncServiceImpl.

---

Outside diff comments:
In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/auth/controller/AuthController.java`:
- Line 185: Remove the debug System.out.println statement that prints "===== HIT
GOOGLE COMPLETE REGISTER =====" from the AuthController. Instead of using
System.out.println for debugging, replace it with proper logging using the
appropriate logger (such as SLF4J or Log4J) that is likely already configured in
the project, or simply remove it if it is no longer needed for functionality.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/dashboard/service/impl/TopicServiceImpl.java`:
- Around line 161-244: Remove all System.out.println debug statements from the
calculateCitationDecay method in TopicServiceImpl and replace them with proper
SLF4J logger calls at appropriate log levels. The statements to replace include
those logging CitationDecay start, API completion time, empty response, prepare
phase, and calculation completion. Use a logger instance (such as one injected
via Spring or created as a class field) and select appropriate log levels (info
for general flow, debug for detailed timing information) based on the purpose of
each statement.

---

Nitpick comments:
In @.github/workflows/prod_owlreka.yml:
- Line 22: GitHub Actions in this workflow are pinned to version tags (like `@v4`)
instead of full commit SHAs, which creates supply-chain security vulnerabilities
through tag replacement attacks. Update all action references (checkout,
setup-java, upload-artifact, download-artifact, azure/login, and
azure/webapps-deploy) to pin to their full commit SHA format instead, including
an inline comment showing the corresponding version tag for reference. For
example, change `- uses: actions/checkout@v4` to `- uses:
actions/checkout@<full-commit-sha> # v4.x.x`.

In @.idea/dataSources.xml:
- Around line 4-8: Remove the `.idea/dataSources.xml` file from version control
as it contains IntelliJ-specific datasource metadata that is
environment-dependent and should not be committed. Add `.idea/dataSources.xml`
to your `.gitignore` file to prevent this type of IDE configuration from being
tracked in the future. If the file has already been committed, use git rm
--cached to remove it from the repository while keeping it locally on your
machine. This eliminates environment-specific churn and repo noise caused by
developer-machine configurations.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/admin/service/impl/AdminServiceImpl.java`:
- Around line 174-176: Replace the use of `ZoneId.systemDefault()` with a fixed
timezone constant to ensure consistent behavior across deployments. Define a
static final field named `DASHBOARD_ZONE` with `ZoneOffset.UTC` at the class
level in AdminServiceImpl. Then replace the call to `ZoneId.systemDefault()` in
the `getApiUsageLast7Days()` method (line 174-176 anchor) with this constant.
Apply the same replacement to any other occurrences of `ZoneId.systemDefault()`
in the sibling location at lines 243-256.

In
`@scipubtts/src/main/java/com/brotherhood/scipubtts/feed/controller/FeedController.java`:
- Around line 19-25: The sync endpoint in FeedController is currently using the
GET HTTP method for a state-changing operation. Change the `@GetMapping`
annotation on the sync method to `@PostMapping` to properly follow HTTP semantics,
since this operation performs a state-changing action (triggering a daily feed
synchronization) that should not be invoked by prefetching, caching, or web
crawlers.