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

Hubobe 51 be 포토 포인트 #23

Merged
merged 78 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from 60 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
2299c0c
feat: content추가
haroya01 Sep 12, 2024
cff9d55
feat: controller -> resolver로 수정
haroya01 Sep 14, 2024
edfd104
feat: UserPoint 엔티티 정의
haroya01 Sep 15, 2024
88558f8
feat: 이미지 업로드 로직 수정
haroya01 Sep 18, 2024
5301870
feat: 북마크 에러 정의
haroya01 Sep 20, 2024
164fc94
refactor: Place 엔티티 PlaceDocument 로 이름 변경
haroya01 Sep 24, 2024
e1ef165
refactor: point 타입 변경
haroya01 Sep 24, 2024
d2d62b3
feat: MemberPreview dto 에 id 추가
haroya01 Sep 24, 2024
c360da4
feat: 북마크 관련 에러 핸들러 추가
haroya01 Sep 24, 2024
45f7bc5
refactor: MongoMigrationManager sout 제거
haroya01 Sep 24, 2024
07331b8
feat: userPoint 에러 정의
haroya01 Sep 24, 2024
981276e
feat: course book mark api 2개로 이원화
haroya01 Sep 24, 2024
a48e3ee
refactor: photoPoint sout 제거
haroya01 Sep 24, 2024
536b071
feat: userPoint 엔티티 정의
haroya01 Sep 24, 2024
4afb6f3
feat: PostSearchDocument sout 제거
haroya01 Sep 24, 2024
07edda0
feat: userPoint 관련 dto 추가
haroya01 Sep 24, 2024
2899f1e
feat: userPoint 관련 쿼리 추가
haroya01 Sep 24, 2024
44809be
feat: userPoint 관련 로직 추가
haroya01 Sep 24, 2024
da8c713
feat: TrackRecord 매서드 추가
haroya01 Sep 24, 2024
31a66dc
chore: 서브 모듈 업데이트
haroya01 Sep 24, 2024
2d68e4c
feat: FixtureBase 정의
haroya01 Sep 29, 2024
3d63f25
refactor: JwtFilter 엑세스 토큰 로그 제거
haroya01 Sep 29, 2024
e0d42a4
feat: test lombok
haroya01 Sep 29, 2024
4dab2f3
feat: githubaction 환경설정
haroya01 Sep 29, 2024
6ecb458
chore: subModule 업데이트
haroya01 Sep 29, 2024
a1fad86
fix: 잘못 작성된 쿼리 수정
haroya01 Sep 29, 2024
2336708
fix: 누락된 변경 추가
haroya01 Sep 29, 2024
5068c1a
chore: todo 주석 제거
haroya01 Sep 29, 2024
434f4e9
feat: @builder 추가
haroya01 Sep 29, 2024
350044b
feat: UserPointSearch 관련 e2e 테스트 추가
haroya01 Sep 29, 2024
ab6bc87
fix: test yml 변경
haroya01 Sep 29, 2024
f8407d3
refactor: redis 포트 변경
haroya01 Sep 29, 2024
76f4f9b
chore: subModule 업데이트
haroya01 Sep 29, 2024
19ac2d9
refactor: 변경된 환경 세팅 반영
haroya01 Sep 29, 2024
57f327f
test: 실패 테스트 임시로 제거
haroya01 Sep 29, 2024
0a6ed7b
chore: subModule 업데이트
haroya01 Sep 29, 2024
e9aded0
fix: github action 환경 설정
haroya01 Sep 29, 2024
b2988f2
fix: github action 환경 설정
haroya01 Sep 29, 2024
347dceb
fix: github action 환경 설정
haroya01 Sep 29, 2024
628aadf
fix: github action 환경 설정
haroya01 Sep 29, 2024
dbc91e1
chore: github action 환경 설정
haroya01 Sep 29, 2024
f28e9e8
chore: subModule 업데이트
haroya01 Sep 29, 2024
07fa376
fix: github action 환경 설정
haroya01 Sep 29, 2024
cfa7fab
fix: github action 환경 설정
haroya01 Sep 29, 2024
60fed31
fix: github action 환경 설정
haroya01 Sep 29, 2024
8a619a3
fix: github action 환경 설정
haroya01 Sep 29, 2024
1db6eeb
fix: github action 환경 설정
haroya01 Sep 29, 2024
ff2788f
fix: github action 환경 설정
haroya01 Sep 29, 2024
32f7747
fix: github action 환경 설정
haroya01 Sep 29, 2024
0caa402
fix: github action 환경 설정
haroya01 Sep 29, 2024
ecb3d31
fix: github action 환경 설정
haroya01 Sep 29, 2024
4fd17b9
fix: github action 환경 설정
haroya01 Sep 29, 2024
7df35f6
fix: github action 환경 설정
haroya01 Sep 29, 2024
929e067
fix: github action 환경 설정
haroya01 Sep 29, 2024
b6eb01d
fix: github action 환경 설정
haroya01 Sep 29, 2024
2bcf7a2
fix: github action 환경 설정
haroya01 Sep 30, 2024
e72c901
fix: github action 환경 설정
haroya01 Sep 30, 2024
e0312cc
fix: github action 환경 설정
haroya01 Sep 30, 2024
f96319f
fix: github action 환경 설정
haroya01 Sep 30, 2024
fa417b2
fix: github action 환경 설정
haroya01 Oct 1, 2024
ef9ff0e
fix: github action 환경 설정
haroya01 Oct 1, 2024
cf41088
fix: github action 환경 설정
haroya01 Oct 1, 2024
00a2a4c
fix: github action 환경 설정
haroya01 Oct 1, 2024
24e5ecb
fix: github action 환경 설정
haroya01 Oct 1, 2024
20a9e85
fix: github action 환경 설정
haroya01 Oct 1, 2024
900568e
fix: github action 환경 설정
haroya01 Oct 1, 2024
e64bdc7
fix: github action 환경 설정
haroya01 Oct 1, 2024
d2cfa70
fix: github action 환경 설정
haroya01 Oct 1, 2024
d0cc753
fix: github action 환경 설정
haroya01 Oct 1, 2024
746623a
fix: test 에러 수정
haroya01 Oct 1, 2024
6498243
chore: subModule 업데이트
haroya01 Oct 1, 2024
03fcfba
feat: 로그 저장
haroya01 Oct 2, 2024
7431801
test: 테스트 코드 추가
haroya01 Oct 2, 2024
d63159d
chore: github action 환경 설정
haroya01 Oct 2, 2024
3051a72
test: image 생성 fixture추가
haroya01 Oct 3, 2024
7916e8f
refactor: TrackRecord 개선
haroya01 Oct 4, 2024
cc77cd5
test: test 코드 작성
haroya01 Oct 4, 2024
0e68491
test: test 에러 문제 해결
haroya01 Oct 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 150 additions & 21 deletions .github/workflows/opened-pr-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,136 @@ jobs:
test:
runs-on: ubuntu-latest

services:
mysql:
image: mysql:8.0.33
env:
MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }}
MYSQL_ROOT_HOST: '%'
MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}
haroya01 marked this conversation as resolved.
Show resolved Hide resolved
TZ: Asia/Seoul
MYSQL_CHARACTER_SET_SERVER: utf8mb4
MYSQL_COLLATION_SERVER: utf8mb4_unicode_ci
ports:
- 3306:3306
options: >-
--health-cmd="mysqladmin ping"
--health-interval=10s
--health-timeout=5s
--health-retries=3
Comment on lines +16 to +31
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using a non-root user for MySQL in the test environment

While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.

Consider creating a dedicated test user for MySQL:

env:
  MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }}
  MYSQL_USER: ${{ secrets.MYSQL_TEST_USER }}
  MYSQL_PASSWORD: ${{ secrets.MYSQL_TEST_PASSWORD }}
  MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}

Then, update your application configuration to use these non-root credentials.

Comment on lines +15 to +31
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using a non-root user for MySQL in the test environment

While using the root user is convenient for testing, it's a good practice to use a dedicated user with limited privileges, even in test environments. This helps to catch potential permission issues early and aligns better with security best practices.

Consider creating a dedicated test user for MySQL:

env:
  MYSQL_DATABASE: ${{ secrets.MYSQL_DATABASE }}
  MYSQL_USER: ${{ secrets.MYSQL_TEST_USER }}
  MYSQL_PASSWORD: ${{ secrets.MYSQL_TEST_PASSWORD }}
  MYSQL_ROOT_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}

Then, update your application configuration to use these non-root credentials.


redis:
image: redis:latest
ports:
- 6379:6379

elasticsearch:
image: docker.elastic.co/elasticsearch/elasticsearch:8.5.3
env:
discovery.type: single-node
xpack.security.enabled: true
ELASTIC_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}
Comment on lines +38 to +43
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use a separate secret for Elasticsearch password

Currently, the Elasticsearch password is set to the MySQL root password. This practice reduces security by reusing credentials across different services.

Create a separate secret for the Elasticsearch password and update the configuration:

env:
  discovery.type: single-node
  xpack.security.enabled: true
  ELASTIC_PASSWORD: ${{ secrets.ELASTICSEARCH_PASSWORD }}

Don't forget to add the new ELASTICSEARCH_PASSWORD secret to your GitHub repository secrets.

xpack.security.authc.api_key.enabled: true
xpack.security.transport.ssl.enabled: false
ports:
- 9200:9200

mongodb:
image: mongo:latest
env:
MONGO_INITDB_ROOT_USERNAME: ${{ secrets.MONGO_ROOT_USERNAME }}
MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.MONGO_ROOT_PASSWORD }}
ports:
- 27017:27017
Comment on lines +38 to +55
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider using a separate secret for Elasticsearch password

The current configuration uses the MySQL root password for Elasticsearch. While this works, it's generally better to use separate secrets for different services to enhance security and make password rotation easier.

Consider creating a new secret specifically for Elasticsearch and updating the workflow:

- ELASTIC_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}
+ ELASTIC_PASSWORD: ${{ secrets.ELASTICSEARCH_PASSWORD }}

Don't forget to add the new secret to your GitHub repository secrets.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
elasticsearch:
image: docker.elastic.co/elasticsearch/elasticsearch:8.5.3
env:
discovery.type: single-node
xpack.security.enabled: true
ELASTIC_PASSWORD: ${{ secrets.MYSQL_ROOT_PASSWORD }}
xpack.security.authc.api_key.enabled: true
xpack.security.transport.ssl.enabled: false
ports:
- 9200:9200
mongodb:
image: mongo:latest
env:
MONGO_INITDB_ROOT_USERNAME: ${{ secrets.MONGO_ROOT_USERNAME }}
MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.MONGO_ROOT_PASSWORD }}
ports:
- 27017:27017
elasticsearch:
image: docker.elastic.co/elasticsearch/elasticsearch:8.5.3
env:
discovery.type: single-node
xpack.security.enabled: true
ELASTIC_PASSWORD: ${{ secrets.ELASTICSEARCH_PASSWORD }}
xpack.security.authc.api_key.enabled: true
xpack.security.transport.ssl.enabled: false
ports:
- 9200:9200
mongodb:
image: mongo:latest
env:
MONGO_INITDB_ROOT_USERNAME: ${{ secrets.MONGO_ROOT_USERNAME }}
MONGO_INITDB_ROOT_PASSWORD: ${{ secrets.MONGO_ROOT_PASSWORD }}
ports:
- 27017:27017

options: >-
--health-cmd "mongosh --eval 'db.runCommand({ping:1})'"
--health-interval 10s
--health-timeout 5s
--health-retries 5
Comment on lines +56 to +60
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider a more portable health check for MongoDB

The current health check uses mongosh, which might not be available in all environments. A more portable solution would be to use the mongo command-line tool, which is more commonly available.

Update the MongoDB health check to use mongo:

options: >-
  --health-cmd "mongo --eval 'db.runCommand({ping:1})'"
  --health-interval 10s
  --health-timeout 5s
  --health-retries 5

This change ensures better compatibility across different environments.



steps:
- name: Wait for Elasticsearch
run: |
timeout 600 bash -c '
while ! curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} -o /dev/null -w "%{http_code}" http://127.0.0.1:9200 | grep -q "200"; do
echo "Waiting for Elasticsearch..."
sleep 5
done
'

- name: Verify Elasticsearch Health
run: |
curl -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health

- name: Install Elasticsearch plugin
run: |
docker exec $(docker ps -q --filter "name=elasticsearch") \
/usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori
Comment on lines +77 to +80
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential issue in Elasticsearch plugin installation

The current plugin installation step uses command substitution without quotes, which could lead to issues if the container name contains spaces or special characters.

Update the plugin installation command to use proper quoting:

- name: Install Elasticsearch plugin
  run: |
    docker exec "$(docker ps -q --filter "name=elasticsearch")" \
    /usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori

This change ensures that the command works correctly regardless of the container name format.

🧰 Tools
🪛 actionlint

78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting

(shellcheck)


- name: Restart Elasticsearch
run: |
docker restart $(docker ps -q --filter "name=elasticsearch")

- name: Wait for Elasticsearch after restart
run: |
timeout 600 bash -c '
until curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | grep -q "status\":\"[green|yellow]"; do
echo "Waiting for Elasticsearch to be operational..."
sleep 15
done
'

- name: Verify Elasticsearch Health
run: |
curl -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health

Comment on lines +64 to +98
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize Elasticsearch setup and verification process

The current Elasticsearch setup and verification process is thorough but can be streamlined for efficiency. Consider combining the wait, plugin installation, restart, and health check steps into a single step:

- name: Setup and verify Elasticsearch
  run: |
    # Wait for Elasticsearch to be ready
    timeout 600 bash -c '
    until curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} -o /dev/null -w "%{http_code}" http://localhost:9200 | grep -q "200"; do
      echo "Waiting for Elasticsearch..."
      sleep 5
    done
    '
    
    # Install plugin
    docker exec "$(docker ps -q --filter "name=elasticsearch")" \
    /usr/share/elasticsearch/bin/elasticsearch-plugin install --batch analysis-nori
    
    # Restart Elasticsearch
    docker restart "$(docker ps -q --filter "name=elasticsearch")"
    
    # Wait for Elasticsearch to be operational after restart
    timeout 600 bash -c '
    until curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | grep -q "status\":\"[green|yellow]"; do
      echo "Waiting for Elasticsearch to be operational..."
      sleep 15
    done
    '
    
    # Verify final health
    curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | jq .

This combined step reduces the number of separate actions while maintaining the necessary checks and setup procedures.

🧰 Tools
🪛 actionlint

78-78: shellcheck reported issue in this script: SC2046:warning:1:13: Quote this to prevent word splitting

(shellcheck)


83-83: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting

(shellcheck)

- name: MySQL connection health check
run: |
for i in {30..0}; do
if mysqladmin ping -h 127.0.0.1 --silent; then
break
fi
echo 'MySQL을 기다리는 중...'
sleep 1
done
if [ $i -eq 0 ]; then
echo 'MySQL이 30초 내에 시작되지 않았습니다.'
exit 1
fi
Comment on lines +99 to +111
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance MySQL health check

The MySQL health check is well-implemented, but there are a few minor improvements that can be made.

Consider the following enhancements:

  1. Fix the quoting issue in the mysqladmin command:

    if mysqladmin ping -h"127.0.0.1" --silent; then
  2. Use English for error messages to ensure clarity for international teams:

    echo 'Waiting for MySQL...'
    # ...
    echo 'MySQL did not start within 30 seconds.'
  3. Add a connection timeout to prevent potential hangs:

    if mysqladmin ping -h"127.0.0.1" --silent --connect_timeout=5; then

These changes will improve the robustness and clarity of the MySQL health check.

🧰 Tools
🪛 actionlint

100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting

(shellcheck)


- name: Verify MySQL connection and configure access
run: |
echo "Showing initial databases:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "SHOW DATABASES;" || echo "Failed to show initial databases"

echo "Creating hubo database:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "CREATE DATABASE IF NOT EXISTS hubo;" || echo "Failed to create hubo database"

echo "Creating root user:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${{ secrets.MYSQL_ROOT_PASSWORD }}';" || echo "Failed to create root user"

echo "Granting privileges:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION;" || echo "Failed to grant privileges"

echo "Flushing privileges:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "FLUSH PRIVILEGES;" || echo "Failed to flush privileges"

echo "Showing final databases:"
mysql --host 127.0.0.1 --port 3306 -uroot -p${{ secrets.MYSQL_ROOT_PASSWORD }} -e "SHOW DATABASES;" || echo "Failed to show final databases"
Comment on lines +113 to +131
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize MySQL setup process

The current MySQL setup process executes multiple commands individually, which can be optimized for efficiency.

Consider combining all MySQL operations into a single connection:

mysql --host 127.0.0.1 --port 3306 -uroot -p"${{ secrets.MYSQL_ROOT_PASSWORD }}" << EOF
SHOW DATABASES;
CREATE DATABASE IF NOT EXISTS hubo;
CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${{ secrets.MYSQL_ROOT_PASSWORD }}';
GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION;
FLUSH PRIVILEGES;
SHOW DATABASES;
EOF

This change will make the setup more efficient while maintaining comprehensive error reporting. Also, consider creating a separate user for the application instead of using root for better security.

Comment on lines +99 to +131
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize MySQL setup and use English for messages

The current MySQL setup process is thorough but can be optimized. Additionally, using English for messages ensures better understanding across international teams.

Consider the following improvements:

  1. Use English for messages:
- echo 'MySQL을 기다리는 중...'
+ echo 'Waiting for MySQL...'
- echo 'MySQL이 30초 내에 시작되지 않았습니다.'
+ echo 'MySQL did not start within 30 seconds.'
  1. Combine MySQL operations into a single connection for efficiency:
- name: Setup and verify MySQL
  run: |
    # Wait for MySQL
    timeout 30 bash -c 'until mysqladmin ping -h"127.0.0.1" -P3306 -uroot -p"${{ secrets.MYSQL_ROOT_PASSWORD }}" --silent; do echo "Waiting for MySQL..."; sleep 1; done'
    
    # Configure MySQL
    mysql -h127.0.0.1 -P3306 -uroot -p"${{ secrets.MYSQL_ROOT_PASSWORD }}" << EOF
    SHOW DATABASES;
    CREATE DATABASE IF NOT EXISTS hubo;
    CREATE USER IF NOT EXISTS 'root'@'%' IDENTIFIED BY '${{ secrets.MYSQL_ROOT_PASSWORD }}';
    GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION;
    FLUSH PRIVILEGES;
    SHOW DATABASES;
EOF

These changes improve efficiency, readability, and internationalization of the MySQL setup process.

🧰 Tools
🪛 actionlint

100-100: shellcheck reported issue in this script: SC2086:info:8:6: Double quote to prevent globbing and word splitting

(shellcheck)

🪛 yamllint

[error] 117-117: trailing spaces

(trailing-spaces)


[error] 120-120: trailing spaces

(trailing-spaces)


[error] 123-123: trailing spaces

(trailing-spaces)


[error] 126-126: trailing spaces

(trailing-spaces)


[error] 129-129: trailing spaces

(trailing-spaces)



- name: Debug Elasticsearch before tests
run: |
echo "Checking Elasticsearch status before running tests..."
curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/health | jq .
echo "Listing Elasticsearch indices..."
curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cat/indices?v
echo "Checking Elasticsearch plugins..."
curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cat/plugins?v
echo "Checking Elasticsearch settings..."
curl -s -u elastic:${{ secrets.MYSQL_ROOT_PASSWORD }} http://127.0.0.1:9200/_cluster/settings?pretty

Comment on lines +134 to +144
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance security in Elasticsearch debug output

While the debug information is valuable for troubleshooting, it might expose sensitive data in CI logs.

Consider the following improvements:

  1. Mask sensitive information in the output:
- name: Debug Elasticsearch before tests
  run: |
    echo "Checking Elasticsearch status before running tests..."
    curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/health | jq 'del(.cluster_name, .cluster_uuid)' | sed 's/\("password":\s*\)"[^"]*"/\1"[REDACTED]"/'
    echo "Listing Elasticsearch indices..."
    curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cat/indices?v | sed 's/\(index\s*\)[^ ]*/\1[REDACTED]/'
    echo "Checking Elasticsearch plugins..."
    curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cat/plugins?v
    echo "Checking Elasticsearch settings..."
    curl -s -u elastic:${{ secrets.ELASTICSEARCH_PASSWORD }} http://localhost:9200/_cluster/settings?pretty | sed 's/\("password":\s*\)"[^"]*"/\1"[REDACTED]"/'
  1. Consider moving detailed debug information to a separate step that only runs on failure or when explicitly requested.

These changes help protect sensitive information while still providing valuable debug output when needed.

- name: Checkout secret code
uses: actions/checkout@v3
with:
Expand Down Expand Up @@ -57,34 +186,34 @@ jobs:
restore-keys: |
${{ runner.os }}-gradle-

- name: Start test environment on remote server
uses: appleboy/ssh-action@master
with:
host: ${{ secrets.BE_HOST }}
username: ${{ secrets.BE_USER }}
port: ${{ secrets.BE_PORT }}
password: ${{ secrets.BE_PW }}
script: |
cd ~/hubo/test
./start-test-docker-compose.sh
# - name: Start test environment on remote server
# uses: appleboy/ssh-action@master
# with:
# host: ${{ secrets.BE_HOST }}
# username: ${{ secrets.BE_USER }}
# port: ${{ secrets.BE_PORT }}
# password: ${{ secrets.BE_PW }}
# script: |
# cd ~/hubo/test
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider removing commented-out remote server actions.

The workflow file contains commented-out steps for starting and stopping a test environment on a remote server. If these steps are no longer needed due to the shift towards a self-contained CI environment, it's best to remove them entirely rather than leaving them as comments.

If you're certain that the remote server actions are no longer required, remove the commented-out code to improve the clarity and maintainability of the workflow file. If there's a possibility that you might need these steps in the future, consider moving them to a separate document or creating a git branch with these changes for future reference.

Also applies to: 146-156

# ./start-test-docker-compose.sh
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove commented-out remote server actions

The presence of commented-out code for remote server actions indicates a shift in testing strategy from remote to local testing. Keeping unused code, even as comments, can lead to confusion and make the workflow file harder to maintain.

Consider removing these commented-out sections entirely to improve the clarity and maintainability of the workflow file. If there's a possibility that you might need these steps in the future, consider moving them to a separate document or creating a git branch with these changes for future reference.

Also applies to: lines 229-239


- name: Grant execute permission for gradlew
run: chmod +x ./gradlew

- name: Test with Gradle
run: ./gradlew test -Dspring.profiles.active=test

- name: Stop test environment on remote server
if: always() # 무조건 실행
uses: appleboy/ssh-action@master
with:
host: ${{ secrets.BE_HOST }}
username: ${{ secrets.BE_USER }}
port: ${{ secrets.BE_PORT }}
password: ${{ secrets.BE_PW }}
script: |
cd ~/hubo/test
./stop-test-docker-compose.sh
# - name: Stop test environment on remote server
# if: always() # 무조건 실행
# uses: appleboy/ssh-action@master
# with:
# host: ${{ secrets.BE_HOST }}
# username: ${{ secrets.BE_USER }}
# port: ${{ secrets.BE_PORT }}
# password: ${{ secrets.BE_PW }}
# script: |
# cd ~/hubo/test
# ./stop-test-docker-compose.sh

- name: Upload coverage reports to Codecov
uses: codecov/codecov-action@v4
Expand Down
2 changes: 1 addition & 1 deletion BE-hubo-gillajabi-resources
6 changes: 6 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ dependencies {

// https://mvnrepository.com/artifact/junit/junit
testImplementation 'junit:junit:4.13.1'
testImplementation 'org.projectlombok:lombok:1.18.28'
testImplementation 'org.projectlombok:lombok:1.18.28'
Comment on lines +66 to +67
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove redundant Lombok test dependencies

The Lombok library is already included as a compile-time dependency (compileOnly and annotationProcessor). Adding it again as a test implementation is unnecessary and redundant. Moreover, it's added twice, which could lead to confusion.

Please remove these lines:

-    testImplementation 'org.projectlombok:lombok:1.18.28'
-    testImplementation 'org.projectlombok:lombok:1.18.28'

The existing Lombok dependencies should be sufficient for both compile-time and test-time usage.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
testImplementation 'org.projectlombok:lombok:1.18.28'
testImplementation 'org.projectlombok:lombok:1.18.28'


compileOnly 'org.projectlombok:lombok'
annotationProcessor 'org.projectlombok:lombok'
Expand Down Expand Up @@ -111,6 +113,10 @@ dependencies {

implementation 'org.springframework.boot:spring-boot-starter-mail'

// https://mvnrepository.com/artifact/org.springframework.security/spring-security-test
implementation 'org.springframework.security:spring-security-test:6.1.9'

Comment on lines +114 to +116
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Adjust Spring Security Test dependency

The Spring Security Test library is typically used for testing purposes and should be a test dependency rather than an implementation dependency. Additionally, it's good practice to align the version with the Spring Boot version used in the project.

Please make the following changes:

  1. Move the dependency to testImplementation.
  2. Use the Spring Boot dependency management to handle the version.

Apply this diff:

-    // https://mvnrepository.com/artifact/org.springframework.security/spring-security-test
-    implementation 'org.springframework.security:spring-security-test:6.1.9'
+    testImplementation 'org.springframework.security:spring-security-test'

This change ensures that:

  1. The Spring Security Test library is only included in the test classpath.
  2. The version is managed by Spring Boot, ensuring compatibility with other Spring components.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// https://mvnrepository.com/artifact/org.springframework.security/spring-security-test
implementation 'org.springframework.security:spring-security-test:6.1.9'
testImplementation 'org.springframework.security:spring-security-test'


}

tasks.register('copyResources', Copy) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
@Table(uniqueConstraints = {@UniqueConstraint(columnNames = {"name", "province"})})
@Builder
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the implications of adding @builder annotation

The addition of the @Builder annotation provides a flexible way to create City objects, which can be beneficial for objects with multiple fields. However, consider the following points:

  1. The class already has a static factory method createCity(). Ensure that the team agrees on when to use the builder pattern versus the factory method to maintain consistency across the codebase.

  2. The @Builder annotation generates an all-args constructor, which might conflict with the existing @AllArgsConstructor. Consider using @Builder(toBuilder = true) to allow creating new instances from existing ones.

  3. If you decide to keep both the builder and the factory method, consider updating the createCity() method to use the builder internally for consistency.

Consider updating the createCity() method to use the builder:

public static City createCity(final CityRequest cityRequest) {
    return City.builder()
               .name(cityRequest.getName())
               .province(cityRequest.getProvince())
               .description(cityRequest.getDescription())
               .build();
}

This change would make the factory method consistent with the new builder pattern.

public class City {

@Id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ public class CoursePreview {
private Long id;
private String name;


public static CoursePreview of(Long courseId, String courseName) {
return new CoursePreview(courseId, courseName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import static org.springframework.http.ResponseEntity.noContent;

@RestController
@RequestMapping("/api/course/bookmark")
@Tag(name = "CourseBookmark", description = "코스 북마크 관련 API")
Expand All @@ -21,10 +23,18 @@ public class CourseBookMarkController {
@Operation(summary = "코스 북마크 토글", description = "북마크를 추가하거나 삭제합니다.")
@PostMapping("/{id}")
@UserOnly
public ResponseEntity<String> toggleCourseBookmark(@PathVariable Long id) {
public ResponseEntity toggleCourseBookmark(@PathVariable Long id) {
String username = SecurityUtil.getCurrentUsername();
courseBookMarkService.toggleCourseBookmark(id, username);
return ResponseEntity.ok().body("북마크를 추가했습니다.");
}
Comment on lines +26 to +30
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve response handling in toggleCourseBookmark

There are a few issues with the current implementation:

  1. The method signature now returns ResponseEntity without a generic type, which reduces type safety.
  2. The success message always indicates that a bookmark has been added, which may be misleading if the bookmark was actually removed.
  3. The method no longer distinguishes between adding and removing a bookmark.

Consider the following improvements:

  1. Restore the generic type in the method signature: ResponseEntity<String>.
  2. Return a more accurate message based on the action performed:
public ResponseEntity<String> toggleCourseBookmark(@PathVariable Long id) {
    String username = SecurityUtil.getCurrentUsername();
    boolean bookmarkAdded = courseBookMarkService.toggleCourseBookmark(id, username);
    String message = bookmarkAdded ? "북마크를 추가했습니다." : "북마크를 제거했습니다.";
    return ResponseEntity.ok().body(message);
}

This approach provides more accurate feedback to the client about the action performed.


@Operation(summary = "코스 북마크 해제", description = "북마크를 제거 합니다")
@DeleteMapping("/{id}")
@UserOnly
public ResponseEntity deleteCourseBookmark(@PathVariable Long id){
String username = SecurityUtil.getCurrentUsername();
boolean isBookmarked = courseBookMarkService.toggleCourseBookmark(id, username);
String message = isBookmarked ? "코스 북마크가 추가되었습니다." : "코스 북마크가 삭제되었습니다.";
return ResponseEntity.ok().body(message);
courseBookMarkService.deleteCourseBookmark(id, username);
return new ResponseEntity(noContent().build().getStatusCode());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,22 @@
import com.hubo.gillajabi.member.domain.entity.Member;
import jakarta.persistence.*;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;
import org.hibernate.annotations.SQLRestriction;

@Getter
@Entity
@Table
@AllArgsConstructor
@NoArgsConstructor
@SQLRestriction(value = "status != 'DELETED'")
@Builder
public class CourseBookMark extends BaseEntity {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;

@ManyToOne
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,32 @@ public class CourseBookMarkService {
private final MemberRepository memberRepository;

@Transactional
public boolean toggleCourseBookmark(Long courseId, String userName) {
public void toggleCourseBookmark(Long courseId, String userName) {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider renaming toggleCourseBookmark to reflect its functionality.

The method toggleCourseBookmark no longer toggles the bookmark status; it only adds a bookmark and throws an exception if it already exists. Renaming the method to addCourseBookmark or createCourseBookmark would make its purpose clearer.

Member member = memberRepository.getEntityByUserName(userName);
Course course = courseRepository.findById(courseId)
.orElseThrow(() -> new EntityNotFoundException("코스 없음"));

boolean existingBookmark = courseBookMarkRepository.existsByMemberAndCourse(member, course);

if (existingBookmark) {
courseBookMarkRepository.deleteByMemberAndCourse(member, course);
return false;
} else {
CourseBookMark newBookmark = new CourseBookMark(member, course);
courseBookMarkRepository.save(newBookmark);
return true;
throw new RuntimeException("이미 북마크 중입니다");
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use a custom exception instead of RuntimeException.

Throwing a general RuntimeException can make error handling less precise. Consider creating a custom exception like BookmarkAlreadyExistsException to provide more specific context.

Example:

+ // Create a custom exception class
+ public class BookmarkAlreadyExistsException extends RuntimeException {
+     public BookmarkAlreadyExistsException(String message) {
+         super(message);
+     }
+ }

...

    if (existingBookmark) {
-       throw new RuntimeException("이미 북마크 중입니다");
+       throw new BookmarkAlreadyExistsException("이미 북마크 중입니다");
    }

Committable suggestion was skipped due to low confidence.

}

CourseBookMark newBookmark = new CourseBookMark(member, course);
courseBookMarkRepository.save(newBookmark);
}

@Transactional
public void deleteCourseBookmark(Long id, String username) {
Member member = memberRepository.getEntityByUserName(username);

CourseBookMark courseBookMark = courseBookMarkRepository.findById(id)
.orElseThrow(() -> new EntityNotFoundException("북마크 없음"));

if (!courseBookMark.getMember().equals(member)) {
throw new IllegalArgumentException("북마크 삭제 권한 없음");
}
Comment on lines +47 to +49
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use a more appropriate exception for unauthorized access.

Throwing IllegalArgumentException may not accurately represent an authorization issue. Consider using AccessDeniedException or creating a custom exception to indicate lack of permission.

Example:

+ import org.springframework.security.access.AccessDeniedException;
...

    if (!courseBookMark.getMember().equals(member)) {
-       throw new IllegalArgumentException("북마크 삭제 권한 없음");
+       throw new AccessDeniedException("북마크 삭제 권한 없음");
    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!courseBookMark.getMember().equals(member)) {
throw new IllegalArgumentException("북마크 삭제 권한 없음");
}
if (!courseBookMark.getMember().equals(member)) {
throw new AccessDeniedException("북마크 삭제 권한 없음");
}


courseBookMark.changeStatusToDeleted();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.hubo.gillajabi.course.infrastructure.exception;

import lombok.Getter;

@Getter
public class CourseBookMarkException extends RuntimeException {

private final int errorCode;

private final String errorMessage;

private final int httpStatus;

public CourseBookMarkException(CourseBookMarkExceptionCode courseBookMarkExceptionCode) {
super(courseBookMarkExceptionCode.getErrorMessage());
this.errorCode = courseBookMarkExceptionCode.getErrorCode();
this.errorMessage = courseBookMarkExceptionCode.getErrorMessage();
this.httpStatus = courseBookMarkExceptionCode.getHttpStatus();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.hubo.gillajabi.course.infrastructure.exception;

import lombok.Getter;

@Getter
public enum CourseBookMarkExceptionCode {

COURSE_BOOK_MARK_NOT_FOUND(1, "북마크를 찾을 수 없습니다.", 404),
COURSE_BOOK_MARK_ALREADY_EXISTS(2, "이미 북마크 중 입니다.", 400);

private final int errorCode;
private final String errorMessage;
private final int httpStatus;

CourseBookMarkExceptionCode(int errorCode, String errorMessage, int httpStatus) {
this.errorCode = errorCode;
this.errorMessage = errorMessage;
this.httpStatus = httpStatus;
}

}
Comment on lines +5 to +21
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding a utility method for exception code lookup.

The enum is well-structured and uses Lombok's @Getter effectively. To enhance its utility, consider adding a static method to retrieve an exception code by its error code. This can be useful when mapping error codes to their corresponding enum constants.

Here's a suggested implementation:

public static CourseBookMarkExceptionCode valueOfCode(int errorCode) {
    for (CourseBookMarkExceptionCode code : values()) {
        if (code.getErrorCode() == errorCode) {
            return code;
        }
    }
    throw new IllegalArgumentException("No matching CourseBookMarkExceptionCode for error code: " + errorCode);
}

This method would allow you to easily retrieve the appropriate exception code given an error code, which can be helpful in error handling and logging scenarios.

Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ public interface CourseBookMarkRepository extends JpaRepository<CourseBookMark,

void deleteByMemberAndCourse(Member member, Course course);

@Query("SELECT cb.course FROM CourseBookMark cb WHERE cb.member = :member")
List<Course> findAllBookMarkedCourse( Member member);

@Query("SELECT cb.id FROM CourseBookMark cb WHERE cb.member = :member")
Set<Long> findAllBookMarkedCourseIds(Member member);

@Query("SELECT cb.course.id FROM CourseBookMark cb WHERE cb.member = :member")
List<Long> findCourseIdsByMember(Member member);
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public class Course extends BaseEntity {
private Long totalRatingSum = 0L;

@OneToMany(mappedBy = "course")
@Builder.Default
private Set<CourseTag> courseTags = new HashSet<>();

@Builder
Expand Down
Loading
Loading