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.

Sign up for GitHub

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

Jump to bottom

Fix doctrine mapping & add integration test that fails on invalid schema #170

Merged
merged 2 commits into from
Jan 13, 2025
Merged

Fix doctrine mapping & add integration test that fails on invalid schema #170

merged 2 commits into from
Jan 13, 2025

Conversation

johanib
Copy link
Contributor

@johanib johanib commented Jan 6, 2025 •
edited
Loading

This change:

  • Adds support for integration/smoke tests
  • Adds an integration test that saves something to the database
  • Add doctrine schema validate to the pipeline
  • Adds a migration that makes the PublicKeyCredentialSource compatible with the upstream webauthn-framework
  • Configure the correct db engine version in doctrine to prevent invalid doctrine mapping

@johanib johanib linked an issue Jan 7, 2025 that may be closed by this pull request
Make migrations compatible with latest webauthn-framework #143
Closed
@johanib johanib force-pushed the feature/update_schema branch 3 times, most recently from b59c8b9 to 77dd85c Compare January 7, 2025 11:01
@johanib johanib changed the title Add smoketest that fails on invalid schema Fix doctrine mapping & add smoketest that fails on invalid schema Jan 7, 2025
@johanib johanib self-assigned this Jan 7, 2025
@johanib johanib force-pushed the feature/update_schema branch 5 times, most recently from c39423b to 8450e99 Compare January 9, 2025 12:25
@johanib johanib marked this pull request as ready for review January 9, 2025 12:26
@johanib johanib force-pushed the feature/update_schema branch from 8450e99 to b2764c4 Compare January 9, 2025 12:34
@johanib johanib requested a review from pablothedude January 9, 2025 12:37
@pablothedude
Copy link
Contributor

Was not sufficient to only add /bin/console doctrine:schema:validate in the GHA script?
Please make this a single commit and replace 'smoketest' with 'integration' in the title.

@johanib johanib changed the title Fix doctrine mapping & add smoketest that fails on invalid schema Fix doctrine mapping & add integration test that fails on invalid schema Jan 13, 2025
@johanib
Fix doctrine mapping & add integration test that fails on invalid schema
ffbfe8f 
Add test that asserts PublicKeyCredentialSource can be saved
Add doctrine schema validate to pipeline

Fix: Implement PublicKeyCredentialSource schema changes

Prior to this change, the PublicKeyCredentialSource entity could not be
saved, because a 3rd party web authn package which we extend
PublicKeyCredentialSource from has new db fields. This resulted in sql
errors.

This change implements the required fields and adds a migration.

Fixes #143
Caused by dd32c26#diff-d2ab9925cad7eac58e0ff4cc0d251a937ecf49e4b6bf57f8b95aab76648a9d34R37

See web-auth/webauthn-framework#592
See https://github.com/web-auth/webauthn-framework/releases/tag/4.8.6
@johanib
Fix doctrine mapping
ba929a4 
Prior to this change, doctrine would detect changes in the entity mapping vs the actual database schema.

```sql
ALTER TABLE users CHANGE icon icon VARCHAR(255) DEFAULT NULL;
ALTER TABLE public_key_credential_sources CHANGE other_ui other_ui LONGTEXT DEFAULT NULL COMMENT '(DC2Type:array)';
```

This changes tells doctrine the correct MySQL version, so it no longer wants to change the database.
@johanib johanib force-pushed the feature/update_schema branch from b2764c4 to ba929a4 Compare January 13, 2025 10:41
@johanib
Copy link
Contributor Author

johanib commented Jan 13, 2025

Was not sufficient to only add /bin/console doctrine:schema:validate in the GHA script? Please make this a single commit and replace 'smoketest' with 'integration' in the title.

Strictly speaking it would be sufficient. But because there is no integration tests, which are now made possible, I added this to see if it works and make a start to add coverage.

I merged the first 2 commits. I left the last one in because it's not related to this ticket.

@johanib johanib requested a review from pablothedude January 13, 2025 10:44
@johanib johanib merged commit 0c7b68b into main Jan 13, 2025
1 check passed
@johanib johanib deleted the feature/update_schema branch January 13, 2025 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phavekes phavekes phavekes approved these changes

@pablothedude pablothedude pablothedude approved these changes

Assignees

@johanib johanib

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make migrations compatible with latest webauthn-framework
3 participants
@johanib @pablothedude @phavekes